Re: Review Request 31471: stout: Create IP and IPNetwork AF_INET abstraction.

[Evelina Dumitrescu <ev...@gmail.com>]

> 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
> 
>