> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 48 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line48> > > > > Why this change? What does an IP storage mean? Please revert it. > > Evelina Dumitrescu wrote: > I prefer storage because we use IP for both address and netmask. IP > address for netmask would not make sense!
removed the storage word. > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 89-93 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line89> > > > > Why do you want to allow this default constructor? > > Evelina Dumitrescu wrote: > sometimes I need to write IP ip and initialize it later. If I don't do > that, it does not compile. replaced with Option<IP>. > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 95-103 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line95> > > > > ``` > > Try<struct in_addr> inAddr() const > > { > > if (family_ != AF_INET) { > > return Error("Not an IPv4 address"); > > } > > > > return storage_.in; > > } > > ``` This situation has been previously discussed and we concluded not to use abreviations. I used in() and in6() instead. > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 189 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line189> > > > > "Failed to parse the IP address" Added "Failed to parse the IP" because we use IP also for address and netmask. > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 191 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line191> > > > > 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)); > > } > > } > > ``` > > Evelina Dumitrescu wrote: > networkToHost is instead of ntohl done > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 232-235 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line232> > > > > Again, you don't need these. These are extremely confusing! > > Evelina Dumitrescu wrote: > why? they are simple wrappers as I explained in previous review requests. removed the networkToHost and hostToNetwork methods. Stored every IP in network order. > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 240 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line240> > > > > ``` > > 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; > > } > > ``` sorry for that > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 242-243 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line242> > > > > Please properly align the arguments. > > Evelina Dumitrescu wrote: > do you ant to pus each one on a separate line ? put every argument on a different line > On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 320-326 > > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line320> > > > > Since we control the contstruction of an IPNetwork, can you move all > > these checks there? 'prefix()' should simply return size_t. > > Evelina Dumitrescu wrote: > Maybe someone will make the family field public/ add a setter and put > different types of families for address and netmask. ok, removed - Evelina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31471/#review74552 ----------------------------------------------------------- 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 > >
