You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/04/04 05:55:55 UTC
Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
Bugs: MESOS-8675
https://issues.apache.org/jira/browse/MESOS-8675
Repository: mesos
Description
-------
After all the CRT APIs were replaced with Windows APIs, we no longer
need to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).
Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE` value. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like
`if (fd < 0)` or `if (fd == -1)`.
With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.
Note that a new enum type `INVALID` was added to support default
construction semantics, and only exists for that reason. An actual
"invalid" handle should be constructed via `int_fd fd = -1` (or
`int_fd fd = INVALID_HANDLE_VALUE` when being explicit on Windows).
Because `int_fd` is now castable to an `int`, and the `FD_CRT` type
was removed, the comparison operators became much simpler.
Diffs
-----
3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
Diff: https://reviews.apache.org/r/66437/diff/1/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On April 4, 2018, 2:49 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/close.hpp
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/66437/diff/1/?file=1992284#file1992284line40>
> >
> > Note that this will cause an SEH exception for `FsTest.Close` when run under a debugger, because it purposefully closes a handle twice. Not sure what we can do about that other than delete that part of the test.
So what do we want to do about the test? Delete that part of the test? Check if we're in a debugger and skip that part of the test?
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review200500
-----------------------------------------------------------
On April 6, 2018, 4:15 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 6, 2018, 4:15 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE` value. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like `if (fd <
> 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
>
>
> Diff: https://reviews.apache.org/r/66437/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review200500
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/close.hpp
Lines 38 (patched)
<https://reviews.apache.org/r/66437/#comment281244>
Note that this will cause an SEH exception for `FsTest.Close` when run under a debugger, because it purposefully closes a handle twice. Not sure what we can do about that other than delete that part of the test.
3rdparty/stout/include/stout/os/windows/fd.hpp
Line 52 (original), 54 (patched)
<https://reviews.apache.org/r/66437/#comment281245>
This part of the `enum` I don't think is necessary. It made sense for default construction, but we can probably get away without it.
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 120-132 (patched)
<https://reviews.apache.org/r/66437/#comment281246>
I am not a fan of this, as it can cause ambigous types to crop up. Working on writing comparison operators that will work without this `operator. int()`
- Andrew Schwartzmeyer
On April 3, 2018, 10:55 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 3, 2018, 10:55 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> need to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE` value. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like
> `if (fd < 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Note that a new enum type `INVALID` was added to support default
> construction semantics, and only exists for that reason. An actual
> "invalid" handle should be constructed via `int_fd fd = -1` (or
> `int_fd fd = INVALID_HANDLE_VALUE` when being explicit on Windows).
>
> Because `int_fd` is now castable to an `int`, and the `FD_CRT` type
> was removed, the comparison operators became much simpler.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
>
>
> Diff: https://reviews.apache.org/r/66437/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review200521
-----------------------------------------------------------
Ship it!
3rdparty/stout/include/stout/os/windows/fd.hpp
Line 50 (original), 52 (patched)
<https://reviews.apache.org/r/66437/#comment281256>
Nice fix. It's so easy to forget about enum classes.
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 128 (patched)
<https://reviews.apache.org/r/66437/#comment281258>
Yep this is weird, but your comment addresses why that's the case. I'm fine with it
- John Kordich
On April 4, 2018, 5:55 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 4, 2018, 5:55 a.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> need to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE` value. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like
> `if (fd < 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Note that a new enum type `INVALID` was added to support default
> construction semantics, and only exists for that reason. An actual
> "invalid" handle should be constructed via `int_fd fd = -1` (or
> `int_fd fd = INVALID_HANDLE_VALUE` when being explicit on Windows).
>
> Because `int_fd` is now castable to an `int`, and the `FD_CRT` type
> was removed, the comparison operators became much simpler.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
>
>
> Diff: https://reviews.apache.org/r/66437/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Akash Gupta <ak...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review200607
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/fd.hpp
Line 57 (original), 58 (patched)
<https://reviews.apache.org/r/66437/#comment281387>
I'm not sure if every default constructed WindowsFD will be an invalid WindowsFD handle. Do you know what happens to the enum and union? I thought C++ doesn't specify anything, so it can have any value if uninitialized. I don't know how these default constructed WindowsFD are used, but you might want to set it to an invalid handle explcitly.
3rdparty/stout/include/stout/os/windows/fd.hpp
Line 82 (original), 72 (patched)
<https://reviews.apache.org/r/66437/#comment281376>
small nit, but you can use the handle constructor here:
`WindowsFD(int crt) : WindowsFD(INVALID_HANDLE_VALUE) { ... }`
3rdparty/stout/include/stout/os/windows/fd.hpp
Line 85 (original), 84 (patched)
<https://reviews.apache.org/r/66437/#comment281377>
Same thing here with using the socket constructor.
- Akash Gupta
On April 5, 2018, 1:58 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 5, 2018, 1:58 a.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE` value. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like `if (fd <
> 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
>
>
> Diff: https://reviews.apache.org/r/66437/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Akash Gupta <ak...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review200763
-----------------------------------------------------------
Ship it!
Ship It!
- Akash Gupta
On April 6, 2018, 11:15 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 6, 2018, 11:15 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE` value. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like `if (fd <
> 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
>
>
> Diff: https://reviews.apache.org/r/66437/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review201472
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/fd.hpp
Line 82 (original), 68 (patched)
<https://reviews.apache.org/r/66437/#comment282857>
Would it make a difference to make this constructor use the `explicit` keyword? If my MSDN searching serves me right, `SOCKET` should be an unsigned int, so we'd at least prevent implicitly casting signed ints into WindowsFDs...
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 163 (patched)
<https://reviews.apache.org/r/66437/#comment282719>
s/soley/solely/
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 226 (patched)
<https://reviews.apache.org/r/66437/#comment282725>
As discussed offline, let's try to keep the set of implemented operators to a minimum. We can add more as they are used.
- Joseph Wu
On April 11, 2018, 7:06 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 11, 2018, 7:06 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like
> `if (fd < 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
> Several unit tests in the `FsTest` suite became cross-platform, with
> the `Close` test being simplified to test against an `int_fd`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
> 3rdparty/stout/tests/os/socket_tests.cpp 8ea0f1238e4293a5cd313b4ee38a920b381f4022
>
>
> Diff: https://reviews.apache.org/r/66437/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review202205
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On April 26, 2018, 9:17 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 26, 2018, 9:17 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like
> `if (fd < 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
> Several unit tests in the `FsTest` suite became cross-platform, with
> the `Close` test being simplified to test against an `int_fd`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/pipe.hpp 365db9480f6258a03ef2e760a19abef8ab177e58
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/socket.hpp 259b05b8c85e399feaccec698d58b7d540cad368
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
> 3rdparty/stout/tests/os/socket_tests.cpp 8ea0f1238e4293a5cd313b4ee38a920b381f4022
>
>
> Diff: https://reviews.apache.org/r/66437/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Akash Gupta <ak...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review202138
-----------------------------------------------------------
Ship it!
Ship It!
- Akash Gupta
On April 27, 2018, 4:17 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 27, 2018, 4:17 a.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like
> `if (fd < 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
> Several unit tests in the `FsTest` suite became cross-platform, with
> the `Close` test being simplified to test against an `int_fd`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/pipe.hpp 365db9480f6258a03ef2e760a19abef8ab177e58
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/socket.hpp 259b05b8c85e399feaccec698d58b7d540cad368
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
> 3rdparty/stout/tests/os/socket_tests.cpp 8ea0f1238e4293a5cd313b4ee38a920b381f4022
>
>
> Diff: https://reviews.apache.org/r/66437/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review202081
-----------------------------------------------------------
Ship it!
Ship It!
- John Kordich
On April 27, 2018, 4:17 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 27, 2018, 4:17 a.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like
> `if (fd < 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
> Several unit tests in the `FsTest` suite became cross-platform, with
> the `Close` test being simplified to test against an `int_fd`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/pipe.hpp 365db9480f6258a03ef2e760a19abef8ab177e58
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/socket.hpp 259b05b8c85e399feaccec698d58b7d540cad368
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
> 3rdparty/stout/tests/os/socket_tests.cpp 8ea0f1238e4293a5cd313b4ee38a920b381f4022
>
>
> Diff: https://reviews.apache.org/r/66437/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/
-----------------------------------------------------------
(Updated April 26, 2018, 9:17 p.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Please review again!
* Added some `static_asserts` to document the underlying types.
* Made the `HANDLE`, `SOCKET`, and `intptr_t` constructors `explicit`.
* Added more commentary.
* Ordered the functions according to the style guide.
* Ran `clang-format` on the buffer.
* Changed the stream output so it can be split on `=` easily.
* Removed ordering operators between two `WindowsFD` types.
* Marked the `std::hash` specialization as `noexcept`.
* Return `std::hash` of the underlying types for consistency.
* Explicitly constructed `int_fd` where now needed.
Bugs: MESOS-8675 and MESOS-8683
https://issues.apache.org/jira/browse/MESOS-8675
https://issues.apache.org/jira/browse/MESOS-8683
Repository: mesos
Description
-------
After all the CRT APIs were replaced with Windows APIs, we no longer
needed to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).
Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like
`if (fd < 0)` or `if (fd == -1)`.
With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.
Thanks to the introduction of the private `int get_valid()` function,
and the removal of the `FD_CRT` type, the comparison operators became
much simpler.
Several unit tests in the `FsTest` suite became cross-platform, with
the `Close` test being simplified to test against an `int_fd`.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
3rdparty/stout/include/stout/os/windows/pipe.hpp 365db9480f6258a03ef2e760a19abef8ab177e58
3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
3rdparty/stout/include/stout/os/windows/socket.hpp 259b05b8c85e399feaccec698d58b7d540cad368
3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
3rdparty/stout/tests/os/socket_tests.cpp 8ea0f1238e4293a5cd313b4ee38a920b381f4022
Diff: https://reviews.apache.org/r/66437/diff/6/
Changes: https://reviews.apache.org/r/66437/diff/5-6/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/
-----------------------------------------------------------
(Updated April 11, 2018, 7:06 p.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Added unit tests and fixed comparing `int_fd(INVALID_SOCKET)` to `int_fd(-1)`.
Bugs: MESOS-8675 and MESOS-8683
https://issues.apache.org/jira/browse/MESOS-8675
https://issues.apache.org/jira/browse/MESOS-8683
Repository: mesos
Description
-------
After all the CRT APIs were replaced with Windows APIs, we no longer
needed to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).
Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like
`if (fd < 0)` or `if (fd == -1)`.
With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.
Thanks to the introduction of the private `int get_valid()` function,
and the removal of the `FD_CRT` type, the comparison operators became
much simpler.
Several unit tests in the `FsTest` suite became cross-platform, with
the `Close` test being simplified to test against an `int_fd`.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
3rdparty/stout/tests/os/socket_tests.cpp 8ea0f1238e4293a5cd313b4ee38a920b381f4022
Diff: https://reviews.apache.org/r/66437/diff/5/
Changes: https://reviews.apache.org/r/66437/diff/4-5/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/
-----------------------------------------------------------
(Updated April 10, 2018, 12:04 p.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Updated file system tests.
Bugs: MESOS-8675 and MESOS-8683
https://issues.apache.org/jira/browse/MESOS-8675
https://issues.apache.org/jira/browse/MESOS-8683
Repository: mesos
Description (updated)
-------
After all the CRT APIs were replaced with Windows APIs, we no longer
needed to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).
Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like
`if (fd < 0)` or `if (fd == -1)`.
With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.
Thanks to the introduction of the private `int get_valid()` function,
and the removal of the `FD_CRT` type, the comparison operators became
much simpler.
Several unit tests in the `FsTest` suite became cross-platform, with
the `Close` test being simplified to test against an `int_fd`.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
Diff: https://reviews.apache.org/r/66437/diff/4/
Changes: https://reviews.apache.org/r/66437/diff/3-4/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/
-----------------------------------------------------------
(Updated April 6, 2018, 4:15 p.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Implemented default ctor to explicitly initialize as an invalid handle.
Bugs: MESOS-8675 and MESOS-8683
https://issues.apache.org/jira/browse/MESOS-8675
https://issues.apache.org/jira/browse/MESOS-8683
Repository: mesos
Description
-------
After all the CRT APIs were replaced with Windows APIs, we no longer
needed to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).
Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE` value. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like `if (fd <
0)` or `if (fd == -1)`.
With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.
Thanks to the introduction of the private `int get_valid()` function,
and the removal of the `FD_CRT` type, the comparison operators became
much simpler.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
Diff: https://reviews.apache.org/r/66437/diff/3/
Changes: https://reviews.apache.org/r/66437/diff/2-3/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/#review200559
-----------------------------------------------------------
Ship it!
This really does feel hacky, but the alternative requires such a significant refactoring that it's probably not worth it (potential bug factory) just to get this working properly on Windows. I'll hold my nose and sign off.
- John Kordich
On April 5, 2018, 1:58 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66437/
> -----------------------------------------------------------
>
> (Updated April 5, 2018, 1:58 a.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8675 and MESOS-8683
> https://issues.apache.org/jira/browse/MESOS-8675
> https://issues.apache.org/jira/browse/MESOS-8683
>
>
> Repository: mesos
>
>
> Description
> -------
>
> After all the CRT APIs were replaced with Windows APIs, we no longer
> needed to support the semantics of an `int` file descriptor in
> general (in the sense of opening a CRT handle that's associated with
> the actual kernel object for the given `HANDLE`). There are specific
> use cases (usually third-party code) which still require a CRT
> int-like file descriptor, which the `crt()` function explicitly
> allocates (this allocation used to be done in the constructor).
>
> Thus the entire `FD_CRT` type was removed from the `WindowsFD`
> abstraction. It still acts like an `int` in the sense that it can be
> constructed from one and compared to one. However, construction via
> `int` only supports the standard file descriptors 0, 1, and 2 for
> `stdin`, `stdout`, and `stderr`. Any other construction creates an
> `int_fd` which holds an `INVALID_HANDLE` value. When being compared to
> an `int`, the abstraction simply returns -1 if it is invalid (based on
> the result of the `is_valid()` method) or 0 if it is valid. This is to
> support the semantics of checking validity by something like `if (fd <
> 0)` or `if (fd == -1)`.
>
> With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
> APIs that switched on the type were simplified, with the last of the
> CRT code deleted.
>
> Thanks to the introduction of the private `int get_valid()` function,
> and the removal of the `FD_CRT` type, the comparison operators became
> much simpler.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
> 3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
> 3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
> 3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
> 3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
> 3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
>
>
> Diff: https://reviews.apache.org/r/66437/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD`
abstraction.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66437/
-----------------------------------------------------------
(Updated April 4, 2018, 6:58 p.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Removed `INVALID` enum, replaced `operator int()` with `private: int get_valid()`.
Bugs: MESOS-8675 and MESOS-8683
https://issues.apache.org/jira/browse/MESOS-8675
https://issues.apache.org/jira/browse/MESOS-8683
Repository: mesos
Description (updated)
-------
After all the CRT APIs were replaced with Windows APIs, we no longer
needed to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).
Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE` value. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like `if (fd <
0)` or `if (fd == -1)`.
With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.
Thanks to the introduction of the private `int get_valid()` function,
and the removal of the `FD_CRT` type, the comparison operators became
much simpler.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/close.hpp ff635e44235d63888a210cd68d49f6678a851e31
3rdparty/stout/include/stout/os/windows/dup.hpp 265046cf7ffc14f7326711d295aa7dd4f0a8a1e3
3rdparty/stout/include/stout/os/windows/fcntl.hpp bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa
3rdparty/stout/include/stout/os/windows/fd.hpp d7f8cdf1ad877eb55589bf5a9e75d295f91990a7
3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
3rdparty/stout/include/stout/os/windows/write.hpp 71006489918d9495d37d2fdfdca08b40b419481a
3rdparty/stout/tests/os/filesystem_tests.cpp c190baa2230298e428d4034b90dccffb59b4e710
Diff: https://reviews.apache.org/r/66437/diff/2/
Changes: https://reviews.apache.org/r/66437/diff/1-2/
Testing
-------
Thanks,
Andrew Schwartzmeyer