----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52544/#review156376 -----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 34 - 39) <https://reviews.apache.org/r/52544/#comment226592> I think we would be better off if we represented the states more accurately according to the invariants of this class. In terms of members: I think we should represent them as: ``` union { // We keep both a CRT FD as well as a `HANDLE` // regardless of whether we were constructed // from a file or a handle. // // This is because once we request for a CRT FD // from a `HANDLE`, we're required to close it // via `_close`. If we were to do the conversion // lazily upon request, the resulting CRT FD // would be dangling. struct { int file; HANDLE handle; }; SOCKET socket; }; ``` I think we should keep separate track of which one is really active. ```cpp enum { FD_FILE, FD_HANDLE, FD_SOCKET } type; ``` 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 42 - 44) <https://reviews.apache.org/r/52544/#comment226593> As suggested above, I think we should keep track of the real active member via the `enum`. While the `isSocket` is fine, due to the invariant that we have both the CRT FD and `HANDLE` set at all times, `isHandle` would inaccurately return `true` for file, and `isFile` would inaccurately return `true` for a handle. 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 46 - 51) <https://reviews.apache.org/r/52544/#comment226594> We should just `= default;` this. 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 53 - 54) <https://reviews.apache.org/r/52544/#comment226595> Also just `= default;` 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 56 - 65) <https://reviews.apache.org/r/52544/#comment226596> With the suggestion above, we get: ``` WindowsFD(HANDLE handle) : type(FD_HANDLE), file( handle == INVALID_HANDLE_VALUE ? -1 : _open_osfhandle(reinterpret_cast<intptr_t>(handle), O_RDWR)), handle(handle) {} ``` 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 67 - 72) <https://reviews.apache.org/r/52544/#comment226597> ``` WindowsFD(SOCKET socket) : type(FD_SOCKET), socket(socket) {} ``` 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 74 - 83) <https://reviews.apache.org/r/52544/#comment226598> ``` WindowsFD(int file) : type(FD_FILE), file(file), handle( file < 0 ? INVALID_HANDLE_VALUE : reinterpret_cast<HANDLE>(_get_osfhandle(file))) {} ``` 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 91 - 113) <https://reviews.apache.org/r/52544/#comment226599> I don't think we need these. ``` WindowsFileDescriptor& operator=(const WindowsFileDescriptor&) = default; ``` should do this for us. That is, `int`, `HANDLE` or `SOCKET` will implicitly converted to `WindowsFileDescriptor` using the implicit constructors above and be assigned. 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 115 - 129) <https://reviews.apache.org/r/52544/#comment226600> My preference would be to remove this member function and define it in `windows/close.hpp` as: ``` Try<Nothing> close(WindowsFD fd) { switch (fd.type) { case WindowsFD::FD_FILE: case WindowsFD::FD_HANDLE: { // The invocation to `_close` also implicitly // closes the underlying `HANDLE`, // therefore we don't need to call `CloseHandle`. ::_close(fd.file); break; } case WindowsFD::FD_SOCKET: { ::shutdown(fd.socket, SD_BOTH); ::closesocket(fd.socket); break; } } return Nothing(); } ``` This would need to be `friend`ed by `WindowsFD`. This also means that all non-member functions also need to be friended, and I think that's fine. ``` friend Try<Nothing> close(WindowsFD fd); ``` 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 318 - 334) <https://reviews.apache.org/r/52544/#comment226601> I feel like we should be able to answer this question (`is_socket`) for `WindowsFD` since we answer it for `int` in POSIX. (Beyond the fact that it seems bizzare for `SOCKET` to somehow __not__ be a socket...) 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 381 - 384) <https://reviews.apache.org/r/52544/#comment226603> Do we actually need this? I think the `int` on the rhs should implicit convert to `WindowsFD` in which case this would be handled by: ``` bool operator==( const os::WindowsFileDescriptor& left, const os::WindowsFileDescriptor& right); ``` 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp (lines 387 - 394) <https://reviews.apache.org/r/52544/#comment226602> My tests show that multiple calls to `_open_osfhandle` with the same handle returns a new `int` every time. `_get_osfhandle` on the other hand returns the same `HANDLE` given the same `int`. This means that this logic here is still correct, but it seems a bit fragile to me. That is, naively changing the order of the handle and file comparisons will yield incorrect behavior. By keeping accurate track of which entity is really active, we can get: ``` inline bool operator==( const os::WindowsFileDescriptor& left, const os::WindowsFileDescriptor& right) { if (left.type != right.type) return false; switch (left.type) { case FD_FILE: { return static_cast<int>(left) == static_cast<int>(right); } case FD_HANDLE: { return static_cast<HANDLE>(left) == static_cast<HANDLE>(right); } case FD_SOCKET: { return static_cast<SOCKET>(left) == static_cast<SOCKET>(right); } } } ``` - Michael Park On Nov. 16, 2016, 11:09 p.m., Daniel Pravat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52544/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2016, 11:09 p.m.) > > > Review request for mesos, Alex Naparu, Alex Clemmer, Joseph Wu, and Michael > Park. > > > Repository: mesos > > > Description > ------- > > In POSIX the `socket`,`pipe` and the `filedescriptor` are > represented by an int type. In Windows a socket is kept in a > `SOCKET` type (64 bit wide), a pipe in a `HANDLE` (64 bit wide) and > a file descriptor in an int. This class unifies all Windows types. > In POSIX this class is an int. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os.hpp > bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba > 3rdparty/stout/include/stout/os/filedescriptor.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/filedescriptor.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/52544/diff/ > > > Testing > ------- > > > Thanks, > > Daniel Pravat > >
