Looks like reviewboard is down, inline below: On Wed, Dec 10, 2014 at 1:19 PM, Dominic Hamon <[email protected]> wrote:
> > > > On Dec. 7, 2014, 9:52 p.m., Ben Mahler wrote: > > > Really appreciate the follow up Evelina, great work! > > > > > > Just looked at style, since I don't have context, I'll leave a few > questions higher level questions for other reviewers to address: > > > > > > (1) Does it makes sense for all of these to be at the top level > "process" namespace? Seems like now that we're introducing a family of > networking calls, perhaps they belong inside a "process::network" namespace? > > > > > > (2) We may want these to be simple wrappers around the syscall and > avoid Try. Any benefit from checking the error code will be lost when using > Try around these. > > > > Evelina Dumitrescu wrote: > > I agree with (1). > > Because it has been brought into discussion the code refactoring > changes from libprocess, I want to suggest some more related to namespace > naming: > > net -> network > > os -> operating_system > > fs -> file_system > > > > I don't quite agree with (2). > > In most cases, we do the same check in every callsite for > success/failure(eg if error < 0). > > If we need something more specific, we can figure it out from the > errno variable. > > i'd rather not change those other namespaces. The short names are > unobtrusive and (i think) understandable. > > i agree with evelina on (2). The whole point of a wrapper is to abstract > away the details of the system call and use our existing error handling > primitives to communicate errors up the stack. > I'm sorry, I should have been more clear :) I didn't intend to suggest changing these, I'm just adding some food for thought: what if the caller needs to distinguish the different "errno" values programmatically? E.g. 1. EINTR --> try again (actually, what to do for EINTR? http://www.madore.org/~david/computers/connect-intr.html) E.g. 2. EFAULT --> fail the program We've run into this only a small number of times, when we do it's a bit painful. But you're right, it is a more general problem (we've gotten pretty far without exposing error kinds in Try). Might want to review the errno values if you haven't already (given the intricacies of EINTR in the link I posted) :) > > > - Dominic > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28789/#review64199 > ----------------------------------------------------------- > > > On Dec. 6, 2014, 5:40 p.m., Evelina Dumitrescu wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/28789/ > > ----------------------------------------------------------- > > > > (Updated Dec. 6, 2014, 5:40 p.m.) > > > > > > Review request for mesos, Ben Mahler and Dominic Hamon. > > > > > > Repository: mesos-git > > > > > > Description > > ------- > > > > Revised comment and improved implementation for socket call wrappers > > > > > > Diffs > > ----- > > > > 3rdparty/libprocess/include/process/socket.hpp > 916576f7b901ab57a03395cef403964a050835b1 > > 3rdparty/libprocess/src/http.cpp > 465e4473685a2e441401e454ad1e8c04db2a4e0a > > 3rdparty/libprocess/src/process.cpp > b87ac2206548815bc992c955252567c131fe6a47 > > 3rdparty/libprocess/src/tests/http_tests.cpp > 3bda76d4b4b958e956b4f5c248190c2fdfcd0d51 > > 3rdparty/libprocess/src/tests/process_tests.cpp > 45be2e15d6f57db89ba7ab758d828ffce3be4b61 > > > > Diff: https://reviews.apache.org/r/28789/diff/ > > > > > > Testing > > ------- > > > > make check > > > > > > Thanks, > > > > Evelina Dumitrescu > > > > > >
