> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 295-297
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line295>
> >
> > Let's introduce this when we actually need it!
I use it in port_mapping.cpp instead of static net::IP LOOPBACK_IP =
net::IP::fromDotDecimal("127.0.0.1/8").get();
> 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.
I prefer storage because we use IP for both address and netmask. IP address for
netmask would not make sense!
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 66
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line66>
> >
> > ```
> > 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_));
> > }
> > ```
ok
> 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?
sometimes I need to write IP ip and initialize it later. If I don't do that, it
does not compile.
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 111-122
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line111>
> >
> > Is this used anywhere? If not, please introduce this once it becomes
> > necessary! Let's keep each patch small and atomic hopefully!
process.cpp
if (isInaddrAny.get() || isLoopback.get()) {
sched.cpp slave.cpp scheduler.cpp master.cpp
if (isLoopback.get()) {
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 143-146
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line143>
> >
> > No need to have the extra parathesis:
> >
> > ```
> > return memcmp(&storage_, &that.storage_, sizeof(storage_)) == 0;
> > ```
ok
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 154-171
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line154>
> >
> > Why this is needed?
Address uses this type of operators.
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 174-181
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line174>
> >
> > 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;
> > ```
ok
> 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));
> > }
> > }
> > ```
networkToHost is instead of ntohl
> 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!
why? they are simple wrappers as I explained in previous review requests.
> 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.
do you ant to pus each one on a separate line ?
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 265-278
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line265>
> >
> > Please introduce it when you actually need it!
yes, I need it for UPID and Address hash_value methods. Otherise, for each of
them I have to duplicate the code.
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 282-283
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line282>
> >
> > Please wrap comments in 70 char width. Here and everywhere else!
I don't understand, the lines don't get over 70 chars.
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 293
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line293>
> >
> > You probably need to pass in the family here.
ok
> 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.
Maybe someone will make the family field public/ add a setter and put different
types of families for address and netmask.
> On Feb. 27, 2015, 8:28 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, lines 333-337
> > <https://reviews.apache.org/r/31471/diff/3/?file=879788#file879788line333>
> >
> > 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!
ok
- 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
>
>
