You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/03/25 06:35:04 UTC

Review Request 19602: Added utility functions to get IPv4 address of a given link device.

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

Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.


Repository: mesos-git


Description
-------

See summary. Will be used by the network isolator.


Diffs
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
  3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 

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


Testing
-------

make check on both OSX and Linux.

Also, manually verify the output with the output from ifconfig/ip addr show


Thanks,

Jie Yu

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/#review38421
-----------------------------------------------------------


Patch looks great!

Reviews applied: [19556, 19602]

All tests passed.

- Mesos ReviewBot


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.

> On March 25, 2014, 5:36 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 384
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line384>
> >
> >     How does getifaddrs() handle multiple IP's on the same interface? At least on Linux it is perfectly valid to have more than one IP addresses, for example one primary and one secondary. I didn't figure this out thus chose to use ioctl() to get primary IP address only.

This happens very rarely and we haven't seen it in real world. Not sure if it is only for Linux because I don't know a way to add an IP address for a link device on OSX.

So, I will just change the comment saying that ip() returns the first available IP address attached to the given link device, and added to TODO to track this problem.


- Jie


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Cong Wang <cw...@twopensource.com>.

> On March 25, 2014, 5:36 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 384
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line384>
> >
> >     How does getifaddrs() handle multiple IP's on the same interface? At least on Linux it is perfectly valid to have more than one IP addresses, for example one primary and one secondary. I didn't figure this out thus chose to use ioctl() to get primary IP address only.
> 
> Jie Yu wrote:
>     This happens very rarely and we haven't seen it in real world. Not sure if it is only for Linux because I don't know a way to add an IP address for a link device on OSX.
>     
>     So, I will just change the comment saying that ip() returns the first available IP address attached to the given link device, and added to TODO to track this problem.

It seems Linux always returns primary IP address first so this should be fine as long as Linux is the only OS supports multiple IP addresses per NIC.


- Cong


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/#review38468
-----------------------------------------------------------



3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/19602/#comment70662>

    How does getifaddrs() handle multiple IP's on the same interface? At least on Linux it is perfectly valid to have more than one IP addresses, for example one primary and one secondary. I didn't figure this out thus chose to use ioctl() to get primary IP address only.


- Cong Wang


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.

> On March 25, 2014, 6:16 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 340
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line340>
> >
> >     s/address_/address()/

Nope! you cannot take the address of an rvalue :)


- Jie


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/#review38478
-----------------------------------------------------------

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/19602/#comment70677>

    s/address_/address()/



3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/19602/#comment70678>

    Can you print ip.address() here?



3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp
<https://reviews.apache.org/r/19602/#comment70680>

    not yours but we should include the headers for these.


- Vinod Kone


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Dominic Hamon <dh...@twopensource.com>.

> On March 25, 2014, 10:31 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 289
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line289>
> >
> >     please call this IPv4, unless you expect us to extend this to also cover IPv6 when we want to support that.
> 
> Jie Yu wrote:
>     The reason I don't call it IPv4 is analogous to:
>     
>     sockaddr_in vs. sockaddr_in6
>     AF_INET vs. AF_INET6
>     ...
>     
>     So, if we want to support IPv6, we can do:
>     
>     inline Result<IP6> ip6(const std::string& name);
>     
>     What do you think?

ok


> On March 25, 2014, 10:31 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 326
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line326>
> >
> >     best practice is to store everything in host order and convert to network order before packing them into a sockaddr_in.
> 
> Jie Yu wrote:
>     Yeah, but turns out that some of our stout functions (e.g., net::getHostname) expect a uint32_t ip in network order. I just want it to be consistent so that we can hook those interfaces together without worrying about converting between host and network order.
>     
>     Also, all the network isolator code expect network order addresses, so that can save us a few conversions:)

well the conversions should be trivial/free depending on the platform so i'm not worried about that. It would be good to open up an issue to convert everything to expect host order except when we're putting it over the wire.


- Dominic


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


On March 24, 2014, 10:35 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 24, 2014, 10:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Chi Zhang <ch...@gmail.com>.

> On March 25, 2014, 5:31 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 326
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line326>
> >
> >     best practice is to store everything in host order and convert to network order before packing them into a sockaddr_in.
> 
> Jie Yu wrote:
>     Yeah, but turns out that some of our stout functions (e.g., net::getHostname) expect a uint32_t ip in network order. I just want it to be consistent so that we can hook those interfaces together without worrying about converting between host and network order.
>     
>     Also, all the network isolator code expect network order addresses, so that can save us a few conversions:)
> 
> Dominic Hamon wrote:
>     well the conversions should be trivial/free depending on the platform so i'm not worried about that. It would be good to open up an issue to convert everything to expect host order except when we're putting it over the wire.

Jie, how wide does this issues exist in stout or other parts of mesos? It might be worth doing a house cleaning earlier than later if it is not already too much involved.

network isolator doesn't really have a preference either, except using what is provided. we just need to convert once for each address type. 


- Chi


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.

> On March 25, 2014, 5:31 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 326
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line326>
> >
> >     best practice is to store everything in host order and convert to network order before packing them into a sockaddr_in.
> 
> Jie Yu wrote:
>     Yeah, but turns out that some of our stout functions (e.g., net::getHostname) expect a uint32_t ip in network order. I just want it to be consistent so that we can hook those interfaces together without worrying about converting between host and network order.
>     
>     Also, all the network isolator code expect network order addresses, so that can save us a few conversions:)
> 
> Dominic Hamon wrote:
>     well the conversions should be trivial/free depending on the platform so i'm not worried about that. It would be good to open up an issue to convert everything to expect host order except when we're putting it over the wire.
> 
> Chi Zhang wrote:
>     Jie, how wide does this issues exist in stout or other parts of mesos? It might be worth doing a house cleaning earlier than later if it is not already too much involved.
>     
>     network isolator doesn't really have a preference either, except using what is provided. we just need to convert once for each address type.

Agreed with Dominic and Chi. I just scanned the code base and tried to make all our code base consistent by storing ip in network order. However, I found that it's a non-trivial and non-portable change. The main issue is that we store ip (in network order) in MasterInfo in ZooKeeper for master detection. Ironically, we store all ports in host order!

So, I created a ticket to track this issue and plan to leave the code as it is:
https://issues.apache.org/jira/browse/MESOS-1145


- Jie


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.

> On March 25, 2014, 5:31 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 289
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line289>
> >
> >     please call this IPv4, unless you expect us to extend this to also cover IPv6 when we want to support that.

The reason I don't call it IPv4 is analogous to:

sockaddr_in vs. sockaddr_in6
AF_INET vs. AF_INET6
...

So, if we want to support IPv6, we can do:

inline Result<IP6> ip6(const std::string& name);

What do you think?


> On March 25, 2014, 5:31 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 326
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line326>
> >
> >     best practice is to store everything in host order and convert to network order before packing them into a sockaddr_in.

Yeah, but turns out that some of our stout functions (e.g., net::getHostname) expect a uint32_t ip in network order. I just want it to be consistent so that we can hook those interfaces together without worrying about converting between host and network order.

Also, all the network isolator code expect network order addresses, so that can save us a few conversions:)


- Jie


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/#review38464
-----------------------------------------------------------



3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/19602/#comment70658>

    please call this IPv4, unless you expect us to extend this to also cover IPv6 when we want to support that.



3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/19602/#comment70660>

    best practice is to store everything in host order and convert to network order before packing them into a sockaddr_in.


- Dominic Hamon


On March 24, 2014, 10:35 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 24, 2014, 10:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.

> On March 25, 2014, 5:11 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 387
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line387>
> >
> >     You can't directly cast ->s_addr to uint32_t, because it is be32, which is big endian. Use ntohl() or just save it as be32.

I've commented in IP class, all addresses are stored in network order.

Is there a type (e.g., be32_t) defined in any system headers that I can use?


- Jie


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.

> On March 25, 2014, 5:11 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 387
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line387>
> >
> >     You can't directly cast ->s_addr to uint32_t, because it is be32, which is big endian. Use ntohl() or just save it as be32.
> 
> Jie Yu wrote:
>     I've commented in IP class, all addresses are stored in network order.
>     
>     Is there a type (e.g., be32_t) defined in any system headers that I can use?
>
> 
> Cong Wang wrote:
>     Essentially it's same type, but tools like sparse will complain about it. I don't think we can store netmask in network order, because we will calculate prefix from it, which means it has to be in host order.

In fact, we store __ip__ in UPID in network order too. We can change the semantics of the public interface in IP to return host order addresses (i.e., return ntonl(address_);), but I don't think that's necessary as the user of this function in anyway need to check the comment to see how to use it.

Also, see line 306, I convert the netmask to host order before I calculate the prefix.


- Jie


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Cong Wang <cw...@twopensource.com>.

> On March 25, 2014, 5:11 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 387
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line387>
> >
> >     You can't directly cast ->s_addr to uint32_t, because it is be32, which is big endian. Use ntohl() or just save it as be32.
> 
> Jie Yu wrote:
>     I've commented in IP class, all addresses are stored in network order.
>     
>     Is there a type (e.g., be32_t) defined in any system headers that I can use?
>

Essentially it's same type, but tools like sparse will complain about it. I don't think we can store netmask in network order, because we will calculate prefix from it, which means it has to be in host order.


- Cong


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Cong Wang <cw...@twopensource.com>.

> On March 25, 2014, 5:11 p.m., Cong Wang wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 387
> > <https://reviews.apache.org/r/19602/diff/1/?file=535060#file535060line387>
> >
> >     You can't directly cast ->s_addr to uint32_t, because it is be32, which is big endian. Use ntohl() or just save it as be32.
> 
> Jie Yu wrote:
>     I've commented in IP class, all addresses are stored in network order.
>     
>     Is there a type (e.g., be32_t) defined in any system headers that I can use?
>
> 
> Cong Wang wrote:
>     Essentially it's same type, but tools like sparse will complain about it. I don't think we can store netmask in network order, because we will calculate prefix from it, which means it has to be in host order.
> 
> Jie Yu wrote:
>     In fact, we store __ip__ in UPID in network order too. We can change the semantics of the public interface in IP to return host order addresses (i.e., return ntonl(address_);), but I don't think that's necessary as the user of this function in anyway need to check the comment to see how to use it.
>     
>     Also, see line 306, I convert the netmask to host order before I calculate the prefix.

Cool! Or use "typedef uint32_t be32;" to make it more obvious. :)


- Cong


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


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/#review38459
-----------------------------------------------------------



3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/19602/#comment70653>

    You can't directly cast ->s_addr to uint32_t, because it is be32, which is big endian. Use ntohl() or just save it as be32.


- Cong Wang


On March 25, 2014, 5:35 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/
-----------------------------------------------------------

(Updated March 25, 2014, 9:46 p.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.


Changes
-------

Updated "depends-on".


Repository: mesos-git


Description
-------

See summary. Will be used by the network isolator.


Diffs
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
  3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 

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


Testing
-------

make check on both OSX and Linux.

Also, manually verify the output with the output from ifconfig/ip addr show


Thanks,

Jie Yu

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/#review38537
-----------------------------------------------------------


Bad patch!

Reviews applied: [19602]

Failed command: git apply --index 19602.patch

Error:
 error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp:20
error: 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp: patch does not apply
error: 3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp: does not exist in index


- Mesos ReviewBot


On March 25, 2014, 9:23 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19602/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 9:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary. Will be used by the network isolator.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19602/diff/
> 
> 
> Testing
> -------
> 
> make check on both OSX and Linux.
> 
> Also, manually verify the output with the output from ifconfig/ip addr show
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/
-----------------------------------------------------------

(Updated March 25, 2014, 9:23 p.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.


Changes
-------

Addressed comments.


Repository: mesos-git


Description
-------

See summary. Will be used by the network isolator.


Diffs (updated)
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
  3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 

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


Testing
-------

make check on both OSX and Linux.

Also, manually verify the output with the output from ifconfig/ip addr show


Thanks,

Jie Yu

Re: Review Request 19602: Added utility functions to get IPv4 address of a given link device.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19602/
-----------------------------------------------------------

(Updated March 25, 2014, 5:35 a.m.)


Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.


Changes
-------

Updated 'depends-on'.


Repository: mesos-git


Description
-------

See summary. Will be used by the network isolator.


Diffs
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp b3d46d6 
  3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp PRE-CREATION 

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


Testing
-------

make check on both OSX and Linux.

Also, manually verify the output with the output from ifconfig/ip addr show


Thanks,

Jie Yu