----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31471/#review74552 -----------------------------------------------------------
I only looked at ip.hpp. Please address the existing comments first and then I'll do a second pass. Thanks! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121147> Why this change? What does an IP storage mean? Please revert it. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121149> s/storage/address/ Here and everywhere else. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121154> ``` explicit IP(const struct in_addr& in) : family_(AF_INET) { clear(); storage_.in = in; } ``` Please add a private helper for 'memset' as it's used in many places: ``` // NOTE: We need to clear the union when creating // an IP because the equality check uses memcmp. void clear() { memset(&storage_, 0, sizeof(storage_)); } ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121166> ``` // Creates an IPv4 address from an unsigned 32-bit 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/#comment121167> Why do you want to allow this default constructor? 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121168> ``` Try<struct in_addr> inAddr() const { if (family_ != AF_INET) { return Error("Not an IPv4 address"); } return storage_.in; } ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121169> Is this used anywhere? If not, please introduce this once it becomes necessary! Let's keep each patch small and atomic hopefully! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121171> Is this used anywhere in this patch? If not, please add it later. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121172> No need to have the extra parathesis: ``` return memcmp(&storage_, &that.storage_, sizeof(storage_)) == 0; ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121173> Why this is needed? 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121165> This is already in class `IP`, therefore the `InternetAddress` seems to be redundant. How about calling it Storage? ``` private: int family_; union Storage { struct in_addr in; // struct in6_addr in6; } storage; ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121174> "Failed to parse the IP address" 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121178> As I said before, we should not introduce this. Here is my suggestion: ``` inline Try<IP> IP::parse(const std::string& value, int family) { if (family == AF_INET) { struct in_addr in; if (inet_pton(AF_INET, value.c_str(), &in) != 1) { return Error("Failed to parse the IP address"); } return IP(in); } else { return Error("Unsupported family: " + stringify(family)); } } ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121180> Again, you don't need these. These are extremely confusing! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121183> ``` case AF_INET: { char buffer[INET_ADDRSTRLEN]; struct in_addr in = ip.inAddr().get(); if (inet_ntop(AF_INET, &in, buffer, sizeof(buffer) == NULL) { // PLEASE DO NOT REMOVE COMMENTS!!!! ABORT(...); } stream << buffer; break; } ``` 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121179> Please properly align the arguments. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121184> Please introduce it when you actually need it! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121185> Please wrap comments in 70 char width. Here and everywhere else! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121187> You probably need to pass in the family here. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121186> Let's introduce this when we actually need it! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121188> Please wrap the comments in 70 char width. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121143> Since we control the contstruction of an IPNetwork, can you move all these checks there? 'prefix()' should simply return size_t. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121189> Why change the impl. here. The purpose of this patch is to add IPv6 capability. Please do not change impl in this patch. Let's do it step by step. THanks! 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121144> Please wrap comments in 70 char width. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121145> The comment here does not seem to add any value. Please kill it. 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp <https://reviews.apache.org/r/31471/#comment121146> Ditto. - Jie Yu On Feb. 27, 2015, 9:22 a.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31471/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2015, 9:22 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/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 > >
