----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31470/#review74309 -----------------------------------------------------------
This is looking great, Evelina! Thanks for all the efforts. No major issue. Please address the following comments and I'll give a shipit! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120860> This does not seem to be necessary because it's the same as the default copy constructor. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120862> s/net mask/netmask/ Here and everywhere else please. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120861> s/(in host order)// s/net mask/netmask/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120863> s/fromAddressNetmask/create/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120864> Adjust the comments here. The function creates an IP network from an IP address and a subnet prefix. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120865> s/fromAddressPrefix/create/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120866> s/(in host order)// 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120867> s/net mask/netmask/ s/(in host order)// 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120868> s/net mask/netmask/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120869> Why return a Try here? Since we control the construction of an IPNetwork, the prefix should always be valid, right? 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120870> s/(in host order)// s/net mask/netmask/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120871> Ditto above. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120872> Ditto. Remove "in host order" and s/net mask/netmask/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120875> The error message here can be improved. How about: ``` return Error("Unexpected number of '/' detected"); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120878> Simply say "Returns the first available IP network of a given link device" 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120876> s/net mask/netmask/ 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120879> You need to callout the semantics when there is an IP but no netmask. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120880> "multiple IP networks" 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120881> "primary IP network" 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31470/#comment120882> There is a leak here, isn't it? You should move 'freeifaddrs' before the if (ip.isError()) check. 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp <https://reviews.apache.org/r/31470/#comment120884> s/ip/network/ 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp <https://reviews.apache.org/r/31470/#comment120887> EXPECT_ERROR(net::fromLinkDevice("non-exist")); 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp <https://reviews.apache.org/r/31470/#comment120888> s/ConstructIPNetmask/ConstructIPNetwork/ - Jie Yu On Feb. 26, 2015, 6:41 a.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31470/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2015, 6:41 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 > 3293086a009a8f7cf7bd343eb7d3e85623636550 > 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp > fb98317a68986cb1228c584a8cd83b07737895a8 > > Diff: https://reviews.apache.org/r/31470/diff/ > > > Testing > ------- > > > Thanks, > > Evelina Dumitrescu > >
