> On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/http.cpp, line 85 > > <https://reviews.apache.org/r/28716/diff/7/?file=783989#file783989line85> > > > > The pattern we use for succinctness and clarity in naming here is: > > > > ``` > > Try<int> connect = process::connect(...); > > ``` > > > > "try" is redundant here since it's encoded in the type. > > > > Ditto everywhere else. :) > > Evelina Dumitrescu wrote: > See dhamon's last commit.
Ah thanks for pointing me to that! That looks better for sure, although I'm hoping to keep it even more consistent with how we've been writing things: ``` Try<int> sock = process::socket(AF_INET, SOCK_STREAM, IPPROTO_IP); ``` I didn't expect to see any "socks" in our code. ;) Interestingly, looks like we leaked this pattern into the linux routing library: ``` ? mesos git:(95752f8) ? grep -R 'sock ' src src/linux/routing/diagnosis/diagnosis.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(NETLINK_INET_DIAG); src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/filter/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/link/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/link/link.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/queueing/internal.hpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); src/linux/routing/route.cpp: Try<Netlink<struct nl_sock> > sock = routing::socket(); ? mesos git:(95752f8) ? grep -R 'sock ' 3rdparty ``` So ideally one of these: ``` Try<int> socket = proces::socket(...); Try<int> s = proces::socket(...); ``` and more generally on the other callsites, in these cases "rc" should have been "error": ``` // Went from 'rc' -> 'accepted', but really we're storing the 'error': int accepted = ::accept(s, (sockaddr*) &addr, &addrlen); // Change to: int error = ::accept(s, (sockaddr*) &addr, &addrlen); ``` ``` // Went from 'rc' -> 'bound', but really we're storing the 'error': int bound = ::bind(s, (sockaddr*) &addr, sizeof(addr)); // Change to: int error = ::bind(s, (sockaddr*) &addr, sizeof(addr)); ``` ``` // Went from 'rc' -> 'connected', but really we're storing the 'error': int connected = ::connect(s, (sockaddr*) &addr, sizeof(addr)); // Change to: int error = ::connect(s, (sockaddr*) &addr, sizeof(addr)); ``` Could you follow up on each of these Evelina? Thank you!! I'll comment on the other Try cases in https://reviews.apache.org/r/28789/ :) > On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/include/process/socket.hpp, line 50 > > <https://reviews.apache.org/r/28716/diff/7/?file=783988#file783988line50> > > > > What does 'rc' mean? :) > > > > Please just call it 'accept' or 'result', it's ok to hide the name in > > the former case. > > Evelina Dumitrescu wrote: > Sorry for that. I was used with this convention from other projects(eg: > linux kernel). It's the abreviation from return code. dhamon submitted a fix > for that > https://github.com/apache/mesos/commit/83d90df125f05ab3c4f1b31311f7e5f1c9d6f8c1. No apologies necessary, thanks for following up, it means a lot! :) > On Dec. 6, 2014, 1:55 a.m., Ben Mahler wrote: > > 3rdparty/libprocess/include/process/socket.hpp, line 40 > > <https://reviews.apache.org/r/28716/diff/7/?file=783988#file783988line40> > > > > Hm.. this comment is a bit odd, because it's on top of the first > > method, but not on top of the others. > > > > Mind removing it, or adding something more standalone (i.e. newline): > > > > ``` > > // Socket call wrappers for different address families. > > > > inline ... > > ``` > > Evelina Dumitrescu wrote: > Thank you for pointing me that out! I submitted another review for this > https://reviews.apache.org/r/28789/ . Thank you! - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28716/#review64146 ----------------------------------------------------------- On Dec. 5, 2014, 9:21 p.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28716/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2014, 9:21 p.m.) > > > Review request for mesos and Dominic Hamon. > > > Bugs: MESOS-2177 > https://issues.apache.org/jira/browse/MESOS-2177 > > > Repository: mesos-git > > > Description > ------- > > Created accept, bind, connect and getsockname wrappers in socket.hpp for > different protocol families > > > Diffs > ----- > > 3rdparty/libprocess/include/process/socket.hpp > ab080c154095029c4a01d189a5fd8a178ba6c92e > 3rdparty/libprocess/src/http.cpp b00f33339366f5c06b6f20e38c5ae0c23b8a9358 > 3rdparty/libprocess/src/process.cpp > 4db7d56af710577e08a9d7dbeb92a1f01559401f > 3rdparty/libprocess/src/tests/http_tests.cpp > a90e65f77904da0a45e1cc0cc9889ae69354a1a5 > 3rdparty/libprocess/src/tests/process_tests.cpp > dec62e88ec993433e1a0777593bb2657b43636dc > > Diff: https://reviews.apache.org/r/28716/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Evelina Dumitrescu > >
