> On Feb. 18, 2015, 1:28 a.m., Jie Yu wrote: > > 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? > > Evelina Dumitrescu wrote: > I appreciate a lot that you took a look at my patches! > > If we splitt the patch into IP and IPNetwork, then the netmask from the > IPNetwork class should be optional. > A situation where we need the net mask as optional is fromLinkDevice. > We will end up with the IP and IPNetwork classes representing the same > thing. > > I agree with splitting this patch and introduce the changes step by step. > > Jie Yu wrote: > Can you expand on why fromLinkDevice needs netmask to be optional? > Looking at the original logics, we only create a IPNetwork if both address > and netmask exist. > > Evelina Dumitrescu wrote: > Because an interface may not have a netmask. See this if: > > if (ifa->ifa_netmask != NULL && > ifa->ifa_netmask->sa_family == family) > > If the interface has a netmask, then the IP is constructed in this way: > > Try<IP> ip = IP::fromAddressNetmask(address, netmask); > > Otherwise, it is used the constructor: > > IP ip(address);
>From my point of view, IPNetmask with an optional netmask field is equivalent >with IP. - Evelina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29288/#review72771 ----------------------------------------------------------- On Feb. 21, 2015, 3:21 p.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29288/ > ----------------------------------------------------------- > > (Updated Feb. 21, 2015, 3:21 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 > b464e517bb1e7b6b381c6cd6c0466ed788a82615 > 3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp > 425132e5d7c3770be4a5a39feea5a2f22179b871 > > Diff: https://reviews.apache.org/r/29288/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Evelina Dumitrescu > >
