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