Re: Review Request 66954: Windows: Added overlapped aware io.hpp file for `os::read/write`.

2018-05-07 Thread Andrew Schwartzmeyer 

---
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)


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)


Run `clang-format` on the whole file please :)



3rdparty/stout/include/stout/os/windows/io.hpp
Lines 40 (patched)


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)


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)


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)


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)


`s/there no/there is no`



3rdparty/stout/include/stout/os/windows/io.hpp
Lines 93 (patched)


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)


Why the mix of `switch` and `if`?



3rdparty/stout/include/stout/os/windows/io.hpp
Lines 95-170 (patched)


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)


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)


So this gets written to by `WSARecv`, but we throw it away?



3rdparty/stout/include/stout/os/windows/io.hpp
Lines 187-189 (patched)


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)


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 
>

Re: Review Request 66954: Windows: Added overlapped aware io.hpp file for `os::read/write`.

2018-05-07 Thread Radhika Jandhyala via Review Board 

---
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)


The in params can be const here



3rdparty/stout/include/stout/os/windows/io.hpp
Lines 89 (patched)


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`.

2018-05-07 Thread Radhika Jandhyala via Review Board 

---
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)


Can this be used to return Result 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
> 
>

Review Request 66954: Windows: Added overlapped aware io.hpp file for `os::read/write`.

2018-05-04 Thread Akash Gupta 

---
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