You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Evelina Dumitrescu <ev...@gmail.com> on 2014/12/04 21:14:13 UTC
Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/
-----------------------------------------------------------
Review request for mesos and Dominic Hamon.
Repository: mesos-git
Description
-------
Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
Created initialization wrappers for sockaddr_in and addrinfo
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
Diff: https://reviews.apache.org/r/28717/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by Evelina Dumitrescu <ev...@gmail.com>.
> On Dec. 6, 2014, 2:04 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 147
> > <https://reviews.apache.org/r/28717/diff/3/?file=784041#file784041line147>
> >
> > Let's call this "result" :)
This convention is used everywhere in the code :)
> On Dec. 6, 2014, 2:04 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 187
> > <https://reviews.apache.org/r/28717/diff/3/?file=784041#file784041line187>
> >
> > In general we avoid abbreviated names like "hints", and "results", which are _informative_ as to what they represent. :)
Can you rephrase this, I don't understand what you mean :). I used the same convention as previously:
struct hostent he, *hep; -> struct addrinfo ai, *aip;
- Evelina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/#review64150
-----------------------------------------------------------
On Dec. 5, 2014, 9:26 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28717/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:26 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2178
> https://issues.apache.org/jira/browse/MESOS-2178
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
> Created initialization wrappers for sockaddr_in and addrinfo
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
>
> Diff: https://reviews.apache.org/r/28717/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by Ben Mahler <be...@gmail.com>.
> On Dec. 6, 2014, 2:04 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 147
> > <https://reviews.apache.org/r/28717/diff/3/?file=784041#file784041line147>
> >
> > Let's call this "result" :)
>
> Evelina Dumitrescu wrote:
> This convention is used everywhere in the code :)
Yikes, you're right! It looks like this anti-pattern slipped into the project in a few locations (most notably the linux networking libraries in the slave).
In general we'll avoid things like "err", you can call this "error" or "result", up to you! I've re-opened for a follow up :)
In libprocess and stout, there's only one place that I can see, and it's unrelated (storing "stderr" filename as "err", not many alternatives there):
```
? mesos git:(95752f8) ? grep -R ' err ' 3rdparty
3rdparty/libprocess/src/tests/subprocess_tests.cpp: string err = path::join(os::getcwd(), "stderr");
3rdparty/libprocess/src/tests/subprocess_tests.cpp: string err = path::join(os::getcwd(), "stderr");
```
In mesos, here are the offenders that I see:
```
? mesos git:(95752f8) ? grep -R ' err ' src
src/linux/routing/diagnosis/diagnosis.cpp: int err = idiagnl_msg_alloc_cache(sock.get().get(), family, states, &c);
src/linux/routing/filter/arp.cpp: int err = rtnl_tc_set_kind(TC_CAST(cls.get()), "basic");
src/linux/routing/filter/icmp.cpp: int err = rtnl_tc_set_kind(TC_CAST(cls.get()), "u32");
src/linux/routing/filter/icmp.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/icmp.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/icmp.cpp: int err = rtnl_u32_get_key(
src/linux/routing/filter/internal.hpp: int err = rtnl_tc_set_kind(TC_CAST(act), "mirred");
src/linux/routing/filter/internal.hpp: err = rtnl_basic_add_action(cls.get(), act);
src/linux/routing/filter/internal.hpp: err = rtnl_u32_add_action(cls.get(), act);
src/linux/routing/filter/internal.hpp: err = rtnl_u32_set_cls_terminal(cls.get());
src/linux/routing/filter/internal.hpp: int err = rtnl_tc_set_kind(TC_CAST(act), "mirred");
src/linux/routing/filter/internal.hpp: err = rtnl_basic_add_action(cls.get(), act);
src/linux/routing/filter/internal.hpp: err = rtnl_u32_add_action(cls.get(), act);
src/linux/routing/filter/internal.hpp: int err = rtnl_u32_set_cls_terminal(cls.get());
src/linux/routing/filter/internal.hpp: int err = rtnl_u32_set_cls_terminal(cls.get());
src/linux/routing/filter/internal.hpp: int err = rtnl_cls_alloc_cache(
src/linux/routing/filter/internal.hpp: int err = rtnl_cls_alloc_cache(
src/linux/routing/filter/internal.hpp: int err = rtnl_cls_add(
src/linux/routing/filter/internal.hpp: int err = rtnl_cls_delete(sock.get().get(), cls.get().get(), 0);
src/linux/routing/filter/internal.hpp: int err = rtnl_cls_change(sock.get().get(), newCls.get().get(), 0);
src/linux/routing/filter/ip.cpp: int err = rtnl_tc_set_kind(TC_CAST(cls.get()), "u32");
src/linux/routing/filter/ip.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/ip.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/ip.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/ip.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/ip.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/ip.cpp: err = rtnl_u32_add_key(
src/linux/routing/filter/ip.cpp: int err = rtnl_u32_get_key(
src/linux/routing/internal.hpp: int err = nl_connect(sock.get(), protocol);
src/linux/routing/link/internal.hpp: int err = rtnl_link_alloc_cache(sock.get().get(), AF_UNSPEC, &c);
src/linux/routing/link/internal.hpp: int err = rtnl_link_alloc_cache(sock.get().get(), AF_UNSPEC, &c);
src/linux/routing/link/link.cpp: int err = rtnl_link_veth_add(
src/linux/routing/link/link.cpp: int err = rtnl_link_delete(sock.get().get(), link.get().get());
src/linux/routing/link/link.cpp: if (err == -NLE_OBJ_NOTFOUND || err == -NLE_NODEV) {
src/linux/routing/queueing/ingress.cpp: int err = rtnl_tc_set_kind(TC_CAST(qdisc.get()), "ingress");
src/linux/routing/queueing/internal.hpp: int err = rtnl_qdisc_alloc_cache(sock.get().get(), &c);
src/linux/routing/queueing/internal.hpp: int err = rtnl_qdisc_add(
src/linux/routing/queueing/internal.hpp: int err = rtnl_qdisc_delete(sock.get().get(), qdisc.get().get());
src/linux/routing/route.cpp: int err = rtnl_route_alloc_cache(sock.get().get(), AF_INET, 0, &c);
src/slave/containerizer/external_containerizer.cpp: Try<int> err = os::open(
src/slave/containerizer/fetcher.cpp: Try<int> err = os::open(
```
Note that it's entirely contained within the linux routing library, which points to it being missed in a small set of reviews.
The containerizer example at the bottom is actually different and more confusing, because it's the "fd" that is being returned from os::open, not the "error". :)
Thanks for checking Evelina!
> On Dec. 6, 2014, 2:04 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 187
> > <https://reviews.apache.org/r/28717/diff/3/?file=784041#file784041line187>
> >
> > In general we avoid abbreviated names like "hints", and "results", which are _informative_ as to what they represent. :)
>
> Evelina Dumitrescu wrote:
> Can you rephrase this, I don't understand what you mean :). I used the same convention as previously:
> struct hostent he, *hep; -> struct addrinfo ai, *aip;
Sure! It's hard to tell what "ai" and "aip" represent in the call to `getaddrinfo`, looking at this code I have to find the man page to have any idea:
```
int getaddrinfo(const char *node, const char *service,
const struct addrinfo *hints,
struct addrinfo **res);
```
The signature uses more informative names, which we can borrow to make our code easier to read. If we name these "hint" and "result", I can look at our callsite and infer some meaning:
```
int error = getaddrinfo(hostname.c_str(), NULL, &hint, &result);
```
Sorry, I didn't realize this was not introduced by you! A lot of code in libprocess was quite old, thanks for helping us clean it up Evelina!! :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/#review64150
-----------------------------------------------------------
On Dec. 5, 2014, 9:26 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28717/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:26 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2178
> https://issues.apache.org/jira/browse/MESOS-2178
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
> Created initialization wrappers for sockaddr_in and addrinfo
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
>
> Diff: https://reviews.apache.org/r/28717/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/#review64150
-----------------------------------------------------------
Great work Evelina!
Just some minor code quality items below, could you or Dominic follow up with the fixes? Thank you! :)
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/28717/#comment106597>
Let's call this "result" :)
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/28717/#comment106599>
In general we avoid abbreviated names like "hints", and "results", which are _informative_ as to what they represent. :)
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/28717/#comment106598>
Ditto here.
- Ben Mahler
On Dec. 5, 2014, 9:26 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28717/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:26 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2178
> https://issues.apache.org/jira/browse/MESOS-2178
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
> Created initialization wrappers for sockaddr_in and addrinfo
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
>
> Diff: https://reviews.apache.org/r/28717/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by Evelina Dumitrescu <ev...@gmail.com>.
> On Dec. 9, 2014, 12:15 p.m., James DeFelice wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, line 195
> > <https://reviews.apache.org/r/28717/diff/3/?file=784041#file784041line195>
> >
> > Should freeaddrinfo be called here before returning, to avoid a memory leak?
Yes, it should! Thanks for that!
- Evelina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/#review64364
-----------------------------------------------------------
On Dec. 5, 2014, 9:26 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28717/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:26 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2178
> https://issues.apache.org/jira/browse/MESOS-2178
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
> Created initialization wrappers for sockaddr_in and addrinfo
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
>
> Diff: https://reviews.apache.org/r/28717/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/#review64364
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/28717/#comment107027>
Should freeaddrinfo be called here before returning, to avoid a memory leak?
- James DeFelice
On Dec. 5, 2014, 9:26 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28717/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2014, 9:26 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Bugs: MESOS-2178
> https://issues.apache.org/jira/browse/MESOS-2178
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
> Created initialization wrappers for sockaddr_in and addrinfo
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
>
> Diff: https://reviews.apache.org/r/28717/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 9:26 p.m.)
Review request for mesos and Dominic Hamon.
Bugs: MESOS-2178
https://issues.apache.org/jira/browse/MESOS-2178
Repository: mesos-git
Description
-------
Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
Created initialization wrappers for sockaddr_in and addrinfo
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
Diff: https://reviews.apache.org/r/28717/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by Evelina Dumitrescu <ev...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/
-----------------------------------------------------------
(Updated Dec. 5, 2014, 9:23 p.m.)
Review request for mesos and Dominic Hamon.
Bugs: MESOS-2178
https://issues.apache.org/jira/browse/MESOS-2178
Repository: mesos-git
Description
-------
Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
Created initialization wrappers for sockaddr_in and addrinfo
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
Diff: https://reviews.apache.org/r/28717/diff/
Testing
-------
make check
Thanks,
Evelina Dumitrescu
Re: Review Request 28717: stout: Introduced getIP and created
initialization wrappers for sockaddr_in and addrinfo
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28717/#review63902
-----------------------------------------------------------
Ship it!
Ship It!
- Dominic Hamon
On Dec. 4, 2014, 12:14 p.m., Evelina Dumitrescu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28717/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2014, 12:14 p.m.)
>
>
> Review request for mesos and Dominic Hamon.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Replaced obsolete functions gethostbyname2_r and gethostbyname2 with getaddrinfo and introduced getIP.
> Created initialization wrappers for sockaddr_in and addrinfo
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a992bd9f7caf3abcc2c5e14519ca7e3ac045bb4b
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
>
> Diff: https://reviews.apache.org/r/28717/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Evelina Dumitrescu
>
>