> On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 62 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line62> > > > > `struct in_addr` stores ip address in network order by standard. No > > need to mention that in the comment.
I suggest mentioning that in the comment because the IP(uint32_t _storage) constructor uses host order for the parameter. > 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. > > Evelina Dumitrescu wrote: > ok, I actually wanted to stick to in_addr(without struct). > > Jie Yu wrote: > I prefer 'struct in_addr' for consistency reason because code elsewhere > uses things like: > struct sockaddr_in > struct sockaddr_ll > ... > > Evelina Dumitrescu wrote: > ok, I'll replace them. done > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 86 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line86> > > > > Ditto. No need to mention network order. Also, please use `struct > > in_addr` consistently. see above > 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 > > } > > ``` > > Evelina Dumitrescu wrote: > it's more clear to use isInaddrAny and isLoopback functions > > Jie Yu wrote: > Since you'll have IP::LOOPBACK(AF_INET) anyway, why add an additional > function? > > Evelina Dumitrescu wrote: > It's only in IPNetwork, used to replace a variable in port_mapping. > > Jie Yu wrote: > OK, if you want to keep these two, please return a bool since we control > the construction of an IP. > > ``` > bool isLoopback(); > bool isAny(); > ``` made the methods return bools > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 166 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line166> > > > > No need to mention `in network order`. see above > 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 > > Evelina Dumitrescu wrote: > I put the parameters on different lines each, I don't understand what's > wrong > > Jie Yu wrote: > Either > ``` > ABORT( > "xxx" + > stringify(xxx) + > stringify(xxx)); > ``` > > Or > ``` > ABORT("XXX" + > stringify(xxx) + > stringify(xxx)); > ``` > > see Case 2 in: > > https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#function-definitioninvocation > > Evelina Dumitrescu wrote: > thanks ! done > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 229 > > <https://reviews.apache.org/r/31471/diff/4/?file=882869#file882869line229> > > > > Why do you need to pass in the `seed`? Please follow other hash > > functions: > > ``` > > inline std::size_t hash_value(const IP& ip) > > { > > size_t seed = 0; > > boost::hash_combine(seed, xxx); > > return seed; > > } > > ``` in pid.cpp the seed needs to be initialized with the hash value of the id. > 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); > > ``` > > Evelina Dumitrescu wrote: > what if the family is not AF_INET or AF_INET6 ? > > Jie Yu wrote: > OK, how about LOOPBACK_V4() and LOOPBACK_V6 (same applied to IP). > > Evelina Dumitrescu wrote: > Previously I used loopbackIpv4Network/loopbackIpv6Network and I was > requested to replace them with loopbackIPNetwork(int family). I'll switch > back then. replaced with loopbackIpv4Network/loopbackIpv6Network > 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(); > > ... > > } > > > > ... > > ``` > > Evelina Dumitrescu wrote: > why is it verbose? > > Jie Yu wrote: > you have switch-case several times and you need to do `free` in each > default case! added static Try<IP> create(const struct sockaddr_storage& _storage) > On March 3, 2015, 6:39 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, lines 76-78 > > <https://reviews.apache.org/r/31471/diff/4/?file=882870#file882870line76> > > > > Let's simply add a CHECK here and return a `struct sockaddr_storage` > > here (not `Try`). > > Evelina Dumitrescu wrote: > ok used ABORT - Evelina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31471/#review74987 ----------------------------------------------------------- On March 5, 2015, 4:06 p.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31471/ > ----------------------------------------------------------- > > (Updated March 5, 2015, 4:06 p.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 > >
