----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29288/#review72771 -----------------------------------------------------------
Glad to see this being worked on, Thanks for all the effort! This is a partial review. Let's address the following comments first and then I'll take a second pass. One high level comment is that we should separate this giant patch. Many changes are being mixed together, making it very hard for reviewers to digest. For example, you renamed `ip` to `IP::fromLinkDevice`. I think that can be pulled into a separate patch, right? Also, let's try not to introduce helper functions that are not currently being used. You can leave a TODO there. That can also bring this patch smaller. For example, you don't have to add the implementation for IPv6 case in `IP::fromLinkDevice` because it's not being used yet. You can always leave a TODO and fill the implementation later in a separate patch. That'll make the reviewer's life much better. I would suggest we first refactor `net::IP` and separate the concepts of IP address and IP network (see my comments below). No need to introduce the concept of family (and all IPv6 stuffs) yet in this patch. You need to update all the callsites that use `net::IP` (e.g., the port mapping isolator might want to take `net::IPNetwork` instead of `net::IP` in some cases). Let's start with this first! And then, in the second patch, you can make `net::IP` IPv6 capable. That means you need to store a union. Simply add TODO in those helper functions that you think you need to fill IPv6 implementaion in. Let's try to keep that patch small as well. Then, we'll see what we go from there. Does that sound good to you? 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118944> It's partially my fault, but we really should separate the concepts of IP address and IP network. This is pretty standard in other lauguages. For instance, Python has `IPAddress` and `IPNetwork`. Go has `IP` and `IPNet`. Given that we already have `net::IP`, I would sugguest that we use that to represent a single IP address and introduce `net::IPNetwork` to represent an IP network (with netmask/prefix). Highly suggest that you take a look at the Go and Python APIs: ``` http://golang.org/pkg/net/ https://docs.python.org/3/library/ipaddress.html ``` 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118960> I would suggest rename it to `parse`. We need to have both `IP::parse("10.0.0.1")` and `IPNetwork::parse("10.0.0.1/8")`. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118963> This need to be moved to `IPNetwork`. Suggest rename it to `Try<IPNetwork> IPNetwork::create(const IP& address, const IP& netmask)`. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118965> Ditto above. This should be: ``` Try<IPNetwork> IPNetwork::create(const IP& address, size_t prefix); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118966> Can you remind me why this is needed? 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118843> Looking at the comments of fields `IP::address_` and `IP::netmask_`, looks like we always store addresses in host order. That makes me wondering how this function is possible? 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118982> Is this needed anywhere? If not, let's keep this patch a pure refactor. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118981> See my top level comments. Let's add the IPv6 stuffs later in a different patch! 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118983> The renaming can be pulled into a separate patch. See my top level comments. We can also introduce the family later once we start to fill IPv6 stuffs. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/29288/#comment118980> See my top level comments. Let's implement the IPv6 case in a separate patch. - Jie Yu On Feb. 12, 2015, 8:05 p.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29288/ > ----------------------------------------------------------- > > (Updated Feb. 12, 2015, 8:05 p.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, Jie Yu, Joris Van > Remoortere, and Niklas Nielsen. > > > Bugs: MESOS-1919 > https://issues.apache.org/jira/browse/MESOS-1919 > > > Repository: mesos > > > Description > ------- > > Created the inner class InAddrStorage encapsulated inside the IP class. > The class uses a union with the in_addr and in6_addr fields. > I considered that the The MasterInfo protobuffers should have both an ipv4 > and an ipv6 field. > I intend to use the same Classifiers, addition, removal and update of > container filters, but write different encode/decode functions for IPv4/ICMP > and IPv6/ICMPv6 because the processing of the protocol headers differ. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp > a0210ea6440086246aafe632f86498abbb70719a > 3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp > 425132e5d7c3770be4a5a39feea5a2f22179b871 > > Diff: https://reviews.apache.org/r/29288/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Evelina Dumitrescu > >
