> On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 71 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line71> > > > > By convension, if you store an ipv4 address in uint32_t, it should be > > in host order. So the code here should be: > > ``` > > // Creates an IP from a 32 bit unsigned integer (in host order). > > explicit IP(uint32_t address) > > : family_(AF_INET) > > { > > clear(); > > storage_.in.s_addr = htonl(address); > > } > > ```
using a single constructor that expects the parameter to be in host order will add too much confusion. > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 115-126 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line115> > > > > I am wondering if this function is necessary given that we implemented > > `==` operator. For example: > > > > ``` > > IP ip1 = IP::parse("0.0.0.0"); > > if (ip1 == IP::ANY(AF_INET)) { > > // XXX > > } > > > > IP ip2 = IP::parse("127.0.0.1"); > > if (ip2 == IP::LOOPBACK(AF_INET)) { > > // XXX > > } > > ``` it's more clear to use isInaddrAny and isLoopback functions > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 142-156 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line142> > > > > These are not in the current code. Why do you need those? If you put IP > > in a map, can they be in a hashmap? see the operators defined in Address class. > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 169 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line169> > > > > No need for tailing underscore for `in`. > > ``` > > struct in_addr in; > > ``` this convention was used before. > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 260 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line260> > > > > Why this function returns a Try? Seems to be uncessary because it > > shouldn't fail, right? How about making it like a constant: > > ``` > > static IPNetwork LOOPBACK(int family); > > ``` what if the family is not AF_INET or AF_INET6 ? > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp, lines 82-87 > > <https://reviews.apache.org/r/31471/diff/4/?file=882871#file882871line82> > > > > This fits in one line? no > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 63 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line63> > > > > Please be consistent. You use `in_addr` here but you use `struct > > in_addr` for the field member. > > > > Please use `struct in_addr` consistently throughout the code base. ok, I actually wanted to stick to in_addr(without struct). > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 211-213 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line211> > > > > Fix the indent please I put the parameters on different lines each, I don't understand what's wrong > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 281 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line281> > > > > Since we control the construction of an IP network, let's just return > > an int here and use ABORT in the default case. ok > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 433 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line433> > > > > Can you make this function a static method of class `IPNetwork`? https://reviews.apache.org/r/31472/ https://reviews.apache.org/r/31473/ > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 454-462 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line454> > > > > The code here is a little verbose. I am wondering can we simply this > > code by introducing a `IP::create` function for `struct sockaddr`. > > > > ``` > > Try<IP> IP::create(const struct sockaddr& addr); > > > > IP address = IP::create(ifa->ifa_addr).get(); > > > > if (xxx) { > > IP netmask = IP::create(ifa->ifa_netmask).get(); > > ... > > } > > > > ... > > ``` why is it verbose? - Evelina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31471/#review74987 ----------------------------------------------------------- On March 3, 2015, 11:27 a.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31471/ > ----------------------------------------------------------- > > (Updated March 3, 2015, 11:27 a.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, Jie Yu, Joris Van > Remoortere, and Niklas Nielsen. > > > Repository: mesos > > > Description > ------- > > see summary > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > 0cd7cb526a3a2514b3b54552253dfa8919e948d0 > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp > 9635bbc6f7dae1d75a780069fcc60fb706221053 > 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp > fb98317a68986cb1228c584a8cd83b07737895a8 > > Diff: https://reviews.apache.org/r/31471/diff/ > > > Testing > ------- > > > Thanks, > > Evelina Dumitrescu > >
