----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31471/#review74987 -----------------------------------------------------------
This is looking good! Thanks for the cleanup! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121870> `struct in_addr` stores ip address in network order by standard. No need to mention that in the comment. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121867> 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. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121869> 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); } ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121871> Ditto. No need to mention the network order for `struct in_addr`. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121872> Ditto. No need to mention network order. Also, please use `struct in_addr` consistently. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121876> See my comments below. Wondering if that's necessary given that we have implemented `==` operator. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121875> 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 } ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121878> 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? 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121879> No need to mention `in network order`. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121868> No need for tailing underscore for `in`. ``` struct in_addr in; ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121880> s/IP::// This is a static method of IP. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121881> s/in_addr/struct in_addr/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121882> s/INET_ADDRSTRLEN/sizeof(buffer)/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121883> Fix the indent please 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121884> 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; } ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121885> 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); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121889> Since we control the construction of an IP network, let's just return an int here and use ABORT in the default case. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121892> s/ipv4Netmask/mask/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121895> Can you make this function a static method of class `IPNetwork`? 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121911> 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(); ... } ... ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121898> Please use `struct sockaddr_in` consistently in the code base 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121908> You made the same mistake again here. This could be a memory leak if the family is not supported, right? So I suggest you checking the family early (i.e., before `getifaddrs` is called) and use CHECK below. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121912> No need for this since we control the construction of an IP network. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/31471/#comment121913> Let's simply add a CHECK here and return a `struct sockaddr_storage` here (not `Try`). 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/31471/#comment121916> Let's try to not sneaking in changes like this in patch that's already very big. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/31471/#comment121918> We declare the variable the first time we use it. So please kill this. 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp <https://reviews.apache.org/r/31471/#comment121921> Again, let's check if family is supported at the begining. 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp <https://reviews.apache.org/r/31471/#comment121922> This fits in one line? - Jie Yu 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 > >
