> 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.
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);
- 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
>
>
