Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-17 Thread Michael Park 

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


Ship it!





3rdparty/stout/include/stout/windows/error.hpp (lines 110 - 114)


Will commit with these flipped.


- Michael Park


On Nov. 17, 2016, 1:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 17, 2016, 1:26 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass the error code down to the Error object explicitly, rather than
> setting the per-thread error and using it implicitly.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/os/windows/su.hpp 
> 777140e1139d6eeab20780e8c0d0a273ce6a8125 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/include/stout/windows/fs.hpp 
> 06c46db54e8deebe5e6715d281db699f8c4c5a58 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 7ca0b5dc9793369ea142684e3614e8f33cac64b6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-17 Thread James Peach 

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

(Updated Nov. 17, 2016, 9:26 p.m.)


Review request for mesos, Alex Clemmer and Michael Park.


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


Repository: mesos


Description
---

Pass the error code down to the Error object explicitly, rather than
setting the per-thread error and using it implicitly.


Diffs (updated)
-

  3rdparty/stout/include/stout/errorbase.hpp 
96d395d37cdad706c2f23fdd9caeefc2a33e0541 
  3rdparty/stout/include/stout/os/posix/rmdir.hpp 
5657b5a18aaedf842b7b780ea7eada73118788cb 
  3rdparty/stout/include/stout/os/windows/su.hpp 
777140e1139d6eeab20780e8c0d0a273ce6a8125 
  3rdparty/stout/include/stout/windows/error.hpp 
f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
  3rdparty/stout/include/stout/windows/fs.hpp 
06c46db54e8deebe5e6715d281db699f8c4c5a58 
  3rdparty/stout/include/stout/windows/os.hpp 
7ca0b5dc9793369ea142684e3614e8f33cac64b6 
  3rdparty/stout/tests/error_tests.cpp 25b50141dd9bbf163aa816750210b298456740a8 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-17 Thread James Peach 

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

(Updated Nov. 17, 2016, 7:07 p.m.)


Review request for mesos, Alex Clemmer and Michael Park.


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


Repository: mesos


Description (updated)
---

Pass the error code down to the Error object explicitly, rather than
setting the per-thread error and using it implicitly.


Diffs (updated)
-

  3rdparty/stout/include/stout/errorbase.hpp 
96d395d37cdad706c2f23fdd9caeefc2a33e0541 
  3rdparty/stout/include/stout/os/posix/rmdir.hpp 
5657b5a18aaedf842b7b780ea7eada73118788cb 
  3rdparty/stout/include/stout/os/windows/su.hpp 
777140e1139d6eeab20780e8c0d0a273ce6a8125 
  3rdparty/stout/include/stout/windows/error.hpp 
f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
  3rdparty/stout/include/stout/windows/fs.hpp 
06c46db54e8deebe5e6715d281db699f8c4c5a58 
  3rdparty/stout/include/stout/windows/os.hpp 
7ca0b5dc9793369ea142684e3614e8f33cac64b6 
  3rdparty/stout/tests/error_tests.cpp 25b50141dd9bbf163aa816750210b298456740a8 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-17 Thread James Peach 


> On Nov. 16, 2016, 10:25 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/os/posix/rmdir.hpp, lines 57-58
> > 
> >
> > This is great!
> > 
> > It seems like we have more opportunities to update here:
> > 
> > ```
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > 166:  errno = ENOTCONN;
> > 167-  return ErrnoError();
> > ```
> > ```
> > 3rdparty/libprocess/src/poll_socket.cpp
> > 138-#ifdef __WINDOWS__
> > 139-::WSASetLastError(opt);
> > 140-#else
> > 141:errno = opt;
> > 142-#endif
> > 143-
> > 144-return Failure(SocketError("Failed to connect to " + 
> > stringify(to)));
> > ```
> > ```
> > 3rdparty/stout/include/stout/windows/os.hpp
> > 254:errno = ENOSYS;
> > 255-return ErrnoError(
> > 261:errno = ENOSYS;
> > 262-return ErrnoError(
> > 294:errno = ECHILD;
> > 295-return WindowsError(
> > 305:errno = ECHILD;
> > 306-return WindowsError(
> > 324:errno = ECHILD;
> > 325-return WindowsError(
> > ```
> > ```
> > 3rdparty/stout/include/stout/os/windows/su.hpp
> > 55:  SetLastError(ERROR_NOT_SUPPORTED);
> > 56-  return WindowsError();
> > ```
> > ```
> > 3rdparty/stout/include/stout/windows/fs.hpp
> > 122:::SetLastError(error);
> > 123-return WindowsError(
> > 124-"'fs::list': 'FindNextFile' failed when searching for files 
> > with "
> > 125-"'pattern '" + pattern + "'");
> > ```
> 
> Michael Park wrote:
> Ah, I see that the first two `libevent_ssl_socket.cpp` and 
> `poll_socket.cpp` are covered by https://reviews.apache.org/r/53475/.

The Windows implementation of `os::waitpid` deliberately sets `errno` in an 
attempt to be compatible with the the POSIX version. Callers assume that it 
will set `errno`. I think it is best to leave this alone for now.


- James


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


On Nov. 4, 2016, 11:03 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 4, 2016, 11:03 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support explicit error codes in ErrnoError and SocketError.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-16 Thread Michael Park 


> On Nov. 16, 2016, 2:25 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/windows/error.hpp, lines 108-110
> > 
> >
> > I think we get these inherited from `WindowsErrorBase`, right?
> 
> James Peach wrote:
> Also need to add `WindowsSocketError(DWORD _code)` for consistency.

My bad about the "implicitly inherited"-ness of constructors. I was incorrect 
there. Discussed doing `using WindowsErrorBase::WindowsErrorBase;` to inherit 
the base constructors, but ultimately decided to declare them explicitly. This 
way it also looks consistent with the `ErrnoError` declarations.


- Michael


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


On Nov. 4, 2016, 4:03 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 4, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support explicit error codes in ErrnoError and SocketError.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-16 Thread James Peach 


> On Nov. 16, 2016, 10:25 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/windows/error.hpp, lines 108-110
> > 
> >
> > I think we get these inherited from `WindowsErrorBase`, right?

Also need to add `WindowsSocketError(DWORD _code)` for consistency.


- James


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


On Nov. 4, 2016, 11:03 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 4, 2016, 11:03 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support explicit error codes in ErrnoError and SocketError.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-16 Thread Michael Park 


> On Nov. 16, 2016, 2:25 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/os/posix/rmdir.hpp, lines 57-58
> > 
> >
> > This is great!
> > 
> > It seems like we have more opportunities to update here:
> > 
> > ```
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > 166:  errno = ENOTCONN;
> > 167-  return ErrnoError();
> > ```
> > ```
> > 3rdparty/libprocess/src/poll_socket.cpp
> > 138-#ifdef __WINDOWS__
> > 139-::WSASetLastError(opt);
> > 140-#else
> > 141:errno = opt;
> > 142-#endif
> > 143-
> > 144-return Failure(SocketError("Failed to connect to " + 
> > stringify(to)));
> > ```
> > ```
> > 3rdparty/stout/include/stout/windows/os.hpp
> > 254:errno = ENOSYS;
> > 255-return ErrnoError(
> > 261:errno = ENOSYS;
> > 262-return ErrnoError(
> > 294:errno = ECHILD;
> > 295-return WindowsError(
> > 305:errno = ECHILD;
> > 306-return WindowsError(
> > 324:errno = ECHILD;
> > 325-return WindowsError(
> > ```
> > ```
> > 3rdparty/stout/include/stout/os/windows/su.hpp
> > 55:  SetLastError(ERROR_NOT_SUPPORTED);
> > 56-  return WindowsError();
> > ```
> > ```
> > 3rdparty/stout/include/stout/windows/fs.hpp
> > 122:::SetLastError(error);
> > 123-return WindowsError(
> > 124-"'fs::list': 'FindNextFile' failed when searching for files 
> > with "
> > 125-"'pattern '" + pattern + "'");
> > ```

Ah, I see that the first two `libevent_ssl_socket.cpp` and `poll_socket.cpp` 
are covered by https://reviews.apache.org/r/53475/.


- Michael


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


On Nov. 4, 2016, 4:03 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 4, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support explicit error codes in ErrnoError and SocketError.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

2016-11-16 Thread Michael Park 

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


Fix it, then Ship it!





3rdparty/stout/include/stout/errorbase.hpp (lines 51 - 55)


Can we flip these? Above, we have the ctor that uses `errno` implicitly 
followed by the one that takes an explicit error code.



3rdparty/stout/include/stout/os/posix/rmdir.hpp (line 57)


This is great!

It seems like we have more opportunities to update here:

```
3rdparty/libprocess/src/libevent_ssl_socket.cpp
166:  errno = ENOTCONN;
167-  return ErrnoError();
```
```
3rdparty/libprocess/src/poll_socket.cpp
138-#ifdef __WINDOWS__
139-::WSASetLastError(opt);
140-#else
141:errno = opt;
142-#endif
143-
144-return Failure(SocketError("Failed to connect to " + 
stringify(to)));
```
```
3rdparty/stout/include/stout/windows/os.hpp
254:errno = ENOSYS;
255-return ErrnoError(
261:errno = ENOSYS;
262-return ErrnoError(
294:errno = ECHILD;
295-return WindowsError(
305:errno = ECHILD;
306-return WindowsError(
324:errno = ECHILD;
325-return WindowsError(
```
```
3rdparty/stout/include/stout/os/windows/su.hpp
55:  SetLastError(ERROR_NOT_SUPPORTED);
56-  return WindowsError();
```
```
3rdparty/stout/include/stout/windows/fs.hpp
122:::SetLastError(error);
123-return WindowsError(
124-"'fs::list': 'FindNextFile' failed when searching for files 
with "
125-"'pattern '" + pattern + "'");
```



3rdparty/stout/include/stout/windows/error.hpp (lines 108 - 110)


I think we get these inherited from `WindowsErrorBase`, right?



3rdparty/stout/include/stout/windows/error.hpp (lines 120 - 124)


I think we get these inherited from `WindowsErrorBase`, right?


- Michael Park


On Nov. 4, 2016, 4:03 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 4, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support explicit error codes in ErrnoError and SocketError.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>