Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-29 Thread James Peach 


> On Nov. 20, 2016, 11:31 p.m., James Peach wrote:
> > 3rdparty/libprocess/include/process/address.hpp, line 241
> > 
> >
> > My usual pattern for dealing with socket addresses is to use a union, 
> > which avoids the fiddly memory copies and casting. For example:
> > ```
> > union {
> >   sockaddr sa;
> >   sockaddr_storage storage;
> >   sockaddr_in sin;
> >   sockaddr_in6 sin6;
> >   sockaddr_un sun;
> > } addr = {0};
> > 
> > addr.sin.sin_family = AF_INET;
> > addr.sin.sin_addr = ip.in().get();
> > addr.sin.sin_port = htons(port);
> > return network::Address(addr.storage);
> > ```
> > 
> > This union helps in lots of places to make code cleaner and to avoid 
> > explicit casting.
> > 
> > I'm not a fan of building APIs that accept `sockaddr_storage`, since 
> > that is what `sockaddr` is for, but that could jut be a matter of taste :)
> 
> Benjamin Hindman wrote:
> I like the `union`, thanks! I'm still using `sockaddr_storage` however, 
> because IIUC `sockaddr` is not actually big enough to deal with things like 
> `sockaddr_in6`, hence the introduction of `sockaddr_storage`. I still see 
> `sockaddr` being sufficient when passing by pointer/reference, but not when 
> wanting to copy all the bits. Am I missing something here?

Yes, you would need either the same union or  `sockaddr_storage` to copy the 
bits into.


- James


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


On Nov. 29, 2016, 8:22 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 29, 2016, 8:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/pid.hpp 
> 36453b600c91c2ddc057db15ec5938e6c819260b 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/http.cpp 3f16f293a5c5cd0b31a85efe94cb6f8019543d45 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
>   3rdparty/libprocess/src/pid.cpp f9313cde006dd067be265343eed60412ad6b0b95 
>   3rdparty/libprocess/src/poll_socket.cpp 
> eb7b48713edd30b545d7be95b5d51b0f71bd422a 
>   3rdparty/libprocess/src/process.cpp 
> e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> ea798d00a0d0bf6b6623c39904ab3c1c7fd2e555 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-29 Thread Joseph Wu 

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



A small followup commit:
```
commit 727cbef97a3a1a51abf0d2df9076eac157d35c42
Author: Joseph Wu 
Date:   Tue Nov 29 14:16:21 2016 -0800

CMake: Added socket_tests.cpp to the libprocess tests.

This test was added in: https://reviews.apache.org/r/53460/
```

- Joseph Wu


On Nov. 29, 2016, 12:22 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 29, 2016, 12:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/pid.hpp 
> 36453b600c91c2ddc057db15ec5938e6c819260b 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/http.cpp 3f16f293a5c5cd0b31a85efe94cb6f8019543d45 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
>   3rdparty/libprocess/src/pid.cpp f9313cde006dd067be265343eed60412ad6b0b95 
>   3rdparty/libprocess/src/poll_socket.cpp 
> eb7b48713edd30b545d7be95b5d51b0f71bd422a 
>   3rdparty/libprocess/src/process.cpp 
> e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> ea798d00a0d0bf6b6623c39904ab3c1c7fd2e555 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-29 Thread Benjamin Hindman 

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

(Updated Nov. 29, 2016, 8:22 p.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Also added unix::Address.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
  3rdparty/libprocess/include/process/address.hpp 
04e3155d65f476208fa852e83b79d173b66288fd 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/pid.hpp 
36453b600c91c2ddc057db15ec5938e6c819260b 
  3rdparty/libprocess/include/process/socket.hpp 
f798af7879546d71e8ef4a295c9cf489a70cb61f 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/http.cpp 3f16f293a5c5cd0b31a85efe94cb6f8019543d45 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
  3rdparty/libprocess/src/pid.cpp f9313cde006dd067be265343eed60412ad6b0b95 
  3rdparty/libprocess/src/poll_socket.cpp 
eb7b48713edd30b545d7be95b5d51b0f71bd422a 
  3rdparty/libprocess/src/process.cpp e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
  3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
  3rdparty/libprocess/src/tests/process_tests.cpp 
ea798d00a0d0bf6b6623c39904ab3c1c7fd2e555 
  3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-29 Thread Benjamin Hindman 


> On Nov. 28, 2016, 10:08 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/socket.hpp, line 433
> > 
> >
> > inline is not necessary for template specification? Let's be consistent 
> > here (either use inline for all of them, or none of them).

`inline` is necessary for a completely specialized template definition, but not 
for the deleted function so I'll remove it there but keep it the other places.


- Benjamin


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


On Nov. 28, 2016, 6:15 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 28, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/address.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
>   3rdparty/libprocess/src/poll_socket.cpp 
> eb7b48713edd30b545d7be95b5d51b0f71bd422a 
>   3rdparty/libprocess/src/process.cpp 
> e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-29 Thread Benjamin Hindman 


> On Nov. 20, 2016, 11:31 p.m., James Peach wrote:
> > 3rdparty/libprocess/include/process/address.hpp, line 129
> > 
> >
> > Suggest that this should be `explicit`. There's some risk that strings 
> > like `127.0.0.1` would be coverted implicitly, whereas if you have to 
> > explicitly convert to `unix::Address` this is unlikely.

Agreed, done.


> On Nov. 20, 2016, 11:31 p.m., James Peach wrote:
> > 3rdparty/libprocess/include/process/address.hpp, line 241
> > 
> >
> > My usual pattern for dealing with socket addresses is to use a union, 
> > which avoids the fiddly memory copies and casting. For example:
> > ```
> > union {
> >   sockaddr sa;
> >   sockaddr_storage storage;
> >   sockaddr_in sin;
> >   sockaddr_in6 sin6;
> >   sockaddr_un sun;
> > } addr = {0};
> > 
> > addr.sin.sin_family = AF_INET;
> > addr.sin.sin_addr = ip.in().get();
> > addr.sin.sin_port = htons(port);
> > return network::Address(addr.storage);
> > ```
> > 
> > This union helps in lots of places to make code cleaner and to avoid 
> > explicit casting.
> > 
> > I'm not a fan of building APIs that accept `sockaddr_storage`, since 
> > that is what `sockaddr` is for, but that could jut be a matter of taste :)

I like the `union`, thanks! I'm still using `sockaddr_storage` however, because 
IIUC `sockaddr` is not actually big enough to deal with things like 
`sockaddr_in6`, hence the introduction of `sockaddr_storage`. I still see 
`sockaddr` being sufficient when passing by pointer/reference, but not when 
wanting to copy all the bits. Am I missing something here?


- Benjamin


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


On Nov. 28, 2016, 6:15 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 28, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/address.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
>   3rdparty/libprocess/src/poll_socket.cpp 
> eb7b48713edd30b545d7be95b5d51b0f71bd422a 
>   3rdparty/libprocess/src/process.cpp 
> e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-28 Thread Jie Yu 

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/address.hpp (line 69)


Ditto.



3rdparty/libprocess/include/process/address.hpp (line 80)


Use switch consistently as other functions?



3rdparty/libprocess/include/process/address.hpp (lines 80 - 81)


This won't compile on windows. Looks like AF_UNIX is not even defined on 
windows:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms740506(v=vs.85).aspx

We should properly wrap UNIX with `#ifndef __WINDOWS__`



3rdparty/libprocess/include/process/address.hpp (line 108)


Wrap with `#ifndef __WINDOWS__`



3rdparty/libprocess/include/process/address.hpp (line 129)


+1. We should check on path length here.

Probably introduce a 'create' function?



3rdparty/libprocess/include/process/address.hpp (lines 133 - 140)


+1 on using union as suggested by James.



3rdparty/libprocess/include/process/address.hpp (line 143)


Maybe you can store a `network::Address` here?



3rdparty/libprocess/include/process/address.hpp (line 167)


This is non-POD global constant which might cause trouble during global 
teardown.

We should either use constexpr here (making sure Address's constructor is 
constexpr constructor), or use static method as we did before.



3rdparty/libprocess/include/process/address.hpp (line 169)


Ditto.



3rdparty/libprocess/include/process/address.hpp (lines 223 - 231)


+1 on using a union as suggested by James.



3rdparty/libprocess/include/process/socket.hpp (line 432)


inline is not necessary for template specification? Let's be consistent 
here (either use inline for all of them, or none of them).



3rdparty/libprocess/src/socket.cpp (lines 56 - 59)


`ifndef WINDOWS`



3rdparty/libprocess/src/tests/socket_tests.cpp (line 39)


Looks like this only works on Linux? should we use a socket path in temp 
dir. Maybe use TemporaryDirectoryTest fixture and use 
`path::join(sandbox.get(), "socket")` as the path?



3rdparty/libprocess/src/tests/ssl_tests.cpp (line 51)


wrap with ifdef WINDOWS


- Jie Yu


On Nov. 28, 2016, 6:15 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 28, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/address.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
>   3rdparty/libprocess/src/poll_socket.cpp 
> eb7b48713edd30b545d7be95b5d51b0f71bd422a 
>   3rdparty/libprocess/src/process.cpp 
> e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-27 Thread Benjamin Hindman 

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

(Updated Nov. 28, 2016, 6:15 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Also added unix::Address.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
  3rdparty/libprocess/include/process/address.hpp 
04e3155d65f476208fa852e83b79d173b66288fd 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
f798af7879546d71e8ef4a295c9cf489a70cb61f 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/address.cpp PRE-CREATION 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
5c0929d3d9f5595bd2f343b98b899fd6b06a67b2 
  3rdparty/libprocess/src/poll_socket.cpp 
eb7b48713edd30b545d7be95b5d51b0f71bd422a 
  3rdparty/libprocess/src/process.cpp e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
  3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
  3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-20 Thread Kevin Klues 


> On Nov. 20, 2016, 11:31 p.m., James Peach wrote:
> > 3rdparty/libprocess/include/process/address.hpp, line 138
> > 
> >
> > You should bounds-check the path here since, `sun_path` is only 108 
> > bytes.

Also, this needs to be a `memcpy()`, not a `strncpy()`. If the address begins 
with a `'\0'` to denote a socket in the abstract namespace on linux, using a 
`strncpy()` will not copy over the string since it stops at the first null 
character.


- Kevin


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


On Nov. 4, 2016, 5:30 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 4, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/address.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 21f878ee81db32ad35878ec053c3f2de3637196c 
>   3rdparty/libprocess/src/poll_socket.cpp 
> f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
>   3rdparty/libprocess/src/process.cpp 
> ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-20 Thread James Peach 

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




3rdparty/libprocess/include/process/address.hpp (line 124)


Suggest that this should be `explicit`. There's some risk that strings like 
`127.0.0.1` would be coverted implicitly, whereas if you have to explicitly 
convert to `unix::Address` this is unlikely.



3rdparty/libprocess/include/process/address.hpp (line 133)


You should bounds-check the path here since, `sun_path` is only 108 bytes.



3rdparty/libprocess/include/process/address.hpp (line 224)


My usual pattern for dealing with socket addresses is to use a union, which 
avoids the fiddly memory copies and casting. For example:
```
union {
  sockaddr sa;
  sockaddr_storage storage;
  sockaddr_in sin;
  sockaddr_in6 sin6;
  sockaddr_un sun;
} addr = {0};

addr.sin.sin_family = AF_INET;
addr.sin.sin_addr = ip.in().get();
addr.sin.sin_port = htons(port);
return network::Address(addr.storage);
```

This union helps in lots of places to make code cleaner and to avoid 
explicit casting.

I'm not a fan of building APIs that accept `sockaddr_storage`, since that 
is what `sockaddr` is for, but that could jut be a matter of taste :)



3rdparty/libprocess/include/process/address.hpp (line 257)


The number of temporary copies here seems a little infortunate when we 
could arrange for something like this to work:

```
if (address->family() == UNIX) {
  const sockaddr *sa = addr->sockaddr();
  return unix::Address(sa);
}
```


- James Peach


On Nov. 4, 2016, 5:30 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> ---
> 
> (Updated Nov. 4, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added unix::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 
> 04e3155d65f476208fa852e83b79d173b66288fd 
>   3rdparty/libprocess/include/process/network.hpp 
> 52110667185370a4c92e2fa524819ab1f34bdec9 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 21a0fc45b55a368a21b3e616c751ab43eebd4902 
>   3rdparty/libprocess/src/address.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 21f878ee81db32ad35878ec053c3f2de3637196c 
>   3rdparty/libprocess/src/poll_socket.cpp 
> f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
>   3rdparty/libprocess/src/process.cpp 
> ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 55c8c309571b1892b0acc4d766eda9bb98085a6f 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 
> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-04 Thread Benjamin Hindman 

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

(Updated Nov. 4, 2016, 5:30 p.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Also added unix::Address.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
  3rdparty/libprocess/include/process/address.hpp 
04e3155d65f476208fa852e83b79d173b66288fd 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
f798af7879546d71e8ef4a295c9cf489a70cb61f 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/address.cpp PRE-CREATION 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/poll_socket.cpp 
f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
  3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
  3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-04 Thread Benjamin Hindman 

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

(Updated Nov. 4, 2016, 7:41 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Also added unix::Address.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
  3rdparty/libprocess/include/process/address.hpp 
04e3155d65f476208fa852e83b79d173b66288fd 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
f798af7879546d71e8ef4a295c9cf489a70cb61f 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/address.cpp PRE-CREATION 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/poll_socket.cpp 
f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
  3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
  3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 53460: Refactored network::Address into inet::Address.

2016-11-04 Thread Benjamin Hindman 

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

(Updated Nov. 4, 2016, 7:33 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Also added unix::Address.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
  3rdparty/libprocess/include/process/address.hpp 
04e3155d65f476208fa852e83b79d173b66288fd 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
f798af7879546d71e8ef4a295c9cf489a70cb61f 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/address.cpp PRE-CREATION 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/poll_socket.cpp 
f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
  3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
  3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 53460: Refactored network::Address into inet::Address.

2016-11-03 Thread Benjamin Hindman 

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Also added unix::Address.


Diffs
-

  3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
  3rdparty/libprocess/include/process/address.hpp 
04e3155d65f476208fa852e83b79d173b66288fd 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
f798af7879546d71e8ef4a295c9cf489a70cb61f 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/address.cpp PRE-CREATION 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 

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


Testing
---

make check


Thanks,

Benjamin Hindman