> 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.
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. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29288/#review72771 ----------------------------------------------------------- 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 > >
