You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Akash Gupta <ak...@hotmail.com> on 2018/05/04 17:21:42 UTC
Review Request 66954: Windows: Added overlapped aware io.hpp file for
`os::read/write`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66954/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
Bugs: MESOS-8674
https://issues.apache.org/jira/browse/MESOS-8674
Repository: mesos
Description
-------
Added a helper file for io, because `os::read/write` need to work on
both non-overlapped and overlapped files, so the logic has gotten
more complicated. The new file, `io.hpp`, has all the shared logic.
Diffs
-----
3rdparty/stout/include/Makefile.am f2e60224d9c729497bbcfffbec989c4f355d8f85
3rdparty/stout/include/stout/os/windows/io.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/66954/diff/1/
Testing
-------
Thanks,
Akash Gupta
Re: Review Request 66954: Windows: Added internal windows file for
overlapped helpers.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66954/#review203686
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Schwartzmeyer
On May 4, 2018, 10:21 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66954/
> -----------------------------------------------------------
>
> (Updated May 4, 2018, 10:21 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8670
> https://issues.apache.org/jira/browse/MESOS-8670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper file for handling overlapped objects, because
> `os::read/write` need to work on overlapped files and the logic can be
> shared.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am f2e60224d9c729497bbcfffbec989c4f355d8f85
> 3rdparty/stout/include/stout/internal/windows/overlapped.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66954/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 66954: Windows: Added overlapped aware io.hpp file
for `os::read/write`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66954/#review202574
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 13-27 (patched)
<https://reviews.apache.org/r/66954/#comment284389>
Is this `os::internal` or is it `internal::windows`? I think because it's very Windows-specific that it's the latter, and therefore should probably be under `stout/internal/windows/overlapped.hpp`.
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 25-27 (patched)
<https://reviews.apache.org/r/66954/#comment284405>
Run `clang-format` on the whole file please :)
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 40 (patched)
<https://reviews.apache.org/r/66954/#comment284399>
Can you point the docs that explain this? (And maybe explain the parameters inline; for instance, we're creating a non-inheritable, auto-resetting, non-signaled, unnamed event, but I had to read the docs to figure that out.)
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 41 (patched)
<https://reviews.apache.org/r/66954/#comment284388>
I know Joseph would point this out: don't mention Mesos in Stout.
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 52-53 (patched)
<https://reviews.apache.org/r/66954/#comment284391>
Well that looks like voodoo. May want to quote those docs inline here:
> This is done by specifying a valid event handle for the hEvent member of the OVERLAPPED structure, and setting its low-order bit. A valid event handle whose low-order bit is set keeps I/O completion from being queued to the completion port.
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 67-77 (patched)
<https://reviews.apache.org/r/66954/#comment284402>
You could replace `DWORD error = ::GetLastError();` with just `WindowsError error;` then check `error.code` in the conditional, and `return error` at the end. Just style though.
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 71 (patched)
<https://reviews.apache.org/r/66954/#comment284401>
`s/there no/there is no`
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 93 (patched)
<https://reviews.apache.org/r/66954/#comment284409>
I really wish that strongly typed enums had gotten rid of the need to do this. Alas :(
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 95-100 (patched)
<https://reviews.apache.org/r/66954/#comment284410>
Why the mix of `switch` and `if`?
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 95-170 (patched)
<https://reviews.apache.org/r/66954/#comment284415>
I think readability would be improved if you moved the `HANDLE` and `SOCKET` logic into two new functions that this function then calls appropriately. That way the top-level switch statement is confined to this function, and the two helper functions each only have one logical switch, instead of this function having nested switches.
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 122 (patched)
<https://reviews.apache.org/r/66954/#comment284412>
This reads funny. It's okay to implicitly cast Windows' `TRUE` to a bool.
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 140 (patched)
<https://reviews.apache.org/r/66954/#comment284413>
So this gets written to by `WSARecv`, but we throw it away?
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 187-189 (patched)
<https://reviews.apache.org/r/66954/#comment284421>
Maybe leave a `NOTE` that this specifically cannot reuse `os::read` and `os::write` because they can't handle a `HANDLE` that was opened in overlapped mode.
Speaking of; what are those functions going to do if given an `int_fd` opened with overlapped semantics?
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 224 (patched)
<https://reviews.apache.org/r/66954/#comment284419>
Our type casts are funny. I assume this is to make it consistent with existing stout APIs?
- Andrew Schwartzmeyer
On May 4, 2018, 10:21 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66954/
> -----------------------------------------------------------
>
> (Updated May 4, 2018, 10:21 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8670
> https://issues.apache.org/jira/browse/MESOS-8670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper file for io, because `os::read/write` need to work on
> both non-overlapped and overlapped files, so the logic has gotten
> more complicated. The new file, `io.hpp`, has all the shared logic.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am f2e60224d9c729497bbcfffbec989c4f355d8f85
> 3rdparty/stout/include/stout/os/windows/io.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66954/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 66954: Windows: Added overlapped aware io.hpp file
for `os::read/write`.
Posted by Radhika Jandhyala via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66954/#review202566
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 61 (patched)
<https://reviews.apache.org/r/66954/#comment284377>
The in params can be const here
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 89 (patched)
<https://reviews.apache.org/r/66954/#comment284378>
Param can be const along with data and size
- Radhika Jandhyala
On May 4, 2018, 5:21 p.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66954/
> -----------------------------------------------------------
>
> (Updated May 4, 2018, 5:21 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8670
> https://issues.apache.org/jira/browse/MESOS-8670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper file for io, because `os::read/write` need to work on
> both non-overlapped and overlapped files, so the logic has gotten
> more complicated. The new file, `io.hpp`, has all the shared logic.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am f2e60224d9c729497bbcfffbec989c4f355d8f85
> 3rdparty/stout/include/stout/os/windows/io.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66954/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 66954: Windows: Added overlapped aware io.hpp file
for `os::read/write`.
Posted by Radhika Jandhyala via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66954/#review202504
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/io.hpp
Lines 175 (patched)
<https://reviews.apache.org/r/66954/#comment284374>
Can this be used to return Result<size_t> instead
- Radhika Jandhyala
On May 4, 2018, 5:21 p.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66954/
> -----------------------------------------------------------
>
> (Updated May 4, 2018, 5:21 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8670
> https://issues.apache.org/jira/browse/MESOS-8670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a helper file for io, because `os::read/write` need to work on
> both non-overlapped and overlapped files, so the logic has gotten
> more complicated. The new file, `io.hpp`, has all the shared logic.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am f2e60224d9c729497bbcfffbec989c4f355d8f85
> 3rdparty/stout/include/stout/os/windows/io.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66954/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>