Re: Review Request 46928: Added safety fixes to and tests `os::close`.

2016-05-10 Thread Joris Van Remoortere 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46928/#review132448
---


Ship it!




Ship It!

- Joris Van Remoortere


On May 10, 2016, 8:15 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46928/
> ---
> 
> (Updated May 10, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-5318
> https://issues.apache.org/jira/browse/MESOS-5318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit has three goals:
> 
>   (1) Expand the `os::close` to support closing Windows `HANDLE`
>   objects. This will simplify code in many places, but most notably
>   (and immediately) subprocess, where sharing code between POSIX and
>   Windows implementations is a major priority.
>   (2) Make `os::close` safe on Windows. Unlike its POSIX cousins, the
>   Windows implementation of `::close` is very picky about which file
>   descriptors it will close. If you pass it a `SOCKET` by mistake,
>   for example, the C runtime will throw a structured exception.
>   Since this could potentially halt the process, and since our core
>   socket abstractions (socket.hpp, for example) use `int` to
>   represent sockets, it is worth investing in safety here.
>   (3) Add `os::close` unit tests to verify the safety requirements.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp 
> 8f7318aecff261b803991f7aa24ad869de8c1162 
>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
> 3f3f62769abdbe44b3e37e0ea9e5f59484b52db6 
>   3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
> 269079f316b51e19990d3058c1b9a34060e3559b 
> 
> Diff: https://reviews.apache.org/r/46928/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46928: Added safety fixes to and tests `os::close`.

2016-05-10 Thread Alex Clemmer 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46928/
---

(Updated May 10, 2016, 8:15 a.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Bugs: MESOS-5318
https://issues.apache.org/jira/browse/MESOS-5318


Repository: mesos


Description
---

This commit has three goals:

  (1) Expand the `os::close` to support closing Windows `HANDLE`
  objects. This will simplify code in many places, but most notably
  (and immediately) subprocess, where sharing code between POSIX and
  Windows implementations is a major priority.
  (2) Make `os::close` safe on Windows. Unlike its POSIX cousins, the
  Windows implementation of `::close` is very picky about which file
  descriptors it will close. If you pass it a `SOCKET` by mistake,
  for example, the C runtime will throw a structured exception.
  Since this could potentially halt the process, and since our core
  socket abstractions (socket.hpp, for example) use `int` to
  represent sockets, it is worth investing in safety here.
  (3) Add `os::close` unit tests to verify the safety requirements.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp 
8f7318aecff261b803991f7aa24ad869de8c1162 
  3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
3f3f62769abdbe44b3e37e0ea9e5f59484b52db6 
  3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
269079f316b51e19990d3058c1b9a34060e3559b 

Diff: https://reviews.apache.org/r/46928/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46928: Added safety fixes to and tests `os::close`.

2016-05-09 Thread Alex Clemmer 


> On May 8, 2016, 9:06 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp, 
> > lines 38-47
> > 
> >
> > https://msdn.microsoft.com/en-us/library/s58ftw19.aspx
> > suggests you use C++ try / catch.
> > Why did you choose the use `__try` & `__except`?

A couple reasons.

(1) No specific advantage to using them. Using C++ exceptions is more flexible, 
but Mesos is largely not supposed to throw exceptions, so we don't need the 
extra flexibility. The guidance here (as I understand it) is meant to encourage 
people to turn on the flag that maps SEH to C++ exceptions so they don't have 
crazy code that awkwardly does both. We don't really have this problem.
(2) But, more to the point, I don't want to add the compiler flag that enables 
them if I can avoid it, because we're not supposed to be using exceptions at 
all. I'd rather be extremely clear at every call site that we expect only to 
catch structured exceptions.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46928/#review132182
---


On May 3, 2016, 5:41 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46928/
> ---
> 
> (Updated May 3, 2016, 5:41 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-5318
> https://issues.apache.org/jira/browse/MESOS-5318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit has three goals:
> 
>   (1) Expand the `os::close` to support closing Windows `HANDLE`
>   objects. This will simplify code in many places, but most notably
>   (and immediately) subprocess, where sharing code between POSIX and
>   Windows implementations is a major priority.
>   (2) Make `os::close` safe on Windows. Unlike its POSIX cousins, the
>   Windows implementation of `::close` is very picky about which file
>   descriptors it will close. If you pass it a `SOCKET` by mistake,
>   for example, the C runtime will throw a structured exception.
>   Since this could potentially halt the process, and since our core
>   socket abstractions (socket.hpp, for example) use `int` to
>   represent sockets, it is worth investing in safety here.
>   (3) Add `os::close` unit tests to verify the safety requirements.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp 
> 8f7318aecff261b803991f7aa24ad869de8c1162 
>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
> 3f3f62769abdbe44b3e37e0ea9e5f59484b52db6 
>   3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
> 269079f316b51e19990d3058c1b9a34060e3559b 
> 
> Diff: https://reviews.apache.org/r/46928/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46928: Added safety fixes to and tests `os::close`.

2016-05-08 Thread Joris Van Remoortere 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46928/#review132182
---




3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp (lines 38 
- 47)


https://msdn.microsoft.com/en-us/library/s58ftw19.aspx
suggests you use C++ try / catch.
Why did you choose the use `__try` & `__except`?


- Joris Van Remoortere


On May 3, 2016, 5:41 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46928/
> ---
> 
> (Updated May 3, 2016, 5:41 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-5318
> https://issues.apache.org/jira/browse/MESOS-5318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit has three goals:
> 
>   (1) Expand the `os::close` to support closing Windows `HANDLE`
>   objects. This will simplify code in many places, but most notably
>   (and immediately) subprocess, where sharing code between POSIX and
>   Windows implementations is a major priority.
>   (2) Make `os::close` safe on Windows. Unlike its POSIX cousins, the
>   Windows implementation of `::close` is very picky about which file
>   descriptors it will close. If you pass it a `SOCKET` by mistake,
>   for example, the C runtime will throw a structured exception.
>   Since this could potentially halt the process, and since our core
>   socket abstractions (socket.hpp, for example) use `int` to
>   represent sockets, it is worth investing in safety here.
>   (3) Add `os::close` unit tests to verify the safety requirements.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp 
> 8f7318aecff261b803991f7aa24ad869de8c1162 
>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
> 3f3f62769abdbe44b3e37e0ea9e5f59484b52db6 
>   3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
> 269079f316b51e19990d3058c1b9a34060e3559b 
> 
> Diff: https://reviews.apache.org/r/46928/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>