> On Feb. 27, 2014, 7:03 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/future.hpp, line 1089 > > <https://reviews.apache.org/r/18550/diff/1/?file=505252#file505252line1089> > > > > We're losing information with this, did you want 'CHECK_READY(*this)' > > instead?
This doesn't look resolved? > On Feb. 27, 2014, 7:03 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 151-152 > > <https://reviews.apache.org/r/18550/diff/1/?file=505253#file505253line151> > > > > The 'strings::join' is likely not async signal safe since it does > > allocation. If you'd like to clean up this code it seems like we need to > > improve ABORT. One suggestion: provide an overload to '_abort' which takes > > 10 const char* parameters with defaults set to NULL, then be able to do: > > > > ABORT("Failed to execl '/bin/sh -c ", command.c_str(), "'\n"); > > > > Also, please kill the whitespace here. > > > > Dominic Hamon wrote: > wouldn't i have to do something like > http://stackoverflow.com/questions/3046889/optional-parameters-with-c-macros ? > > maybe in this case it's better to lose the information and just abort > with 'failed to execl'. maybe log the command attempt to info before the > attempt? You could get away with doing something simpler with variadic macros (which we already assume other places). Consider: #define ABORT(...) _Abort(_ABORT_PREFIX)(__VA_ARGS__) struct _Abort { _Abort(const char* _prefix) : prefix(_prefix) {} void operator () ( const char* arg0 = "", const char* arg1 = "", const char* arg2 = "", const char* arg3 = "", const char* arg4 = "", const char* arg5 = "", const char* arg6 = "", const char* arg7 = "", const char* arg8 = "", const char* arg9 = "") { // Write the failure message in an async-signal safe manner, // assuming strlen is async-signal safe or optimized out. // In fact, it is highly unlikely that strlen would be // implemented in an unsafe manner: // http://austingroupbugs.net/view.php?id=692 while (write(STDERR_FILENO, prefix, strlen(prefix)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg1, strlen(arg1)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg2, strlen(arg2)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg3, strlen(arg3)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg4, strlen(arg4)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg5, strlen(arg5)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg6, strlen(arg6)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg7, strlen(arg7)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg8, strlen(arg8)) == -1 && errno == EINTR); while (write(STDERR_FILENO, arg9, strlen(arg9)) == -1 && errno == EINTR); abort(); } const char* prefix; }; Alternatively you could make the defaults be NULL and only write if non-NULL. How about throwing this in a review assuming 18549 and depend on it in this review? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18550/#review35644 ----------------------------------------------------------- On March 6, 2014, 12:28 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18550/ > ----------------------------------------------------------- > > (Updated March 6, 2014, 12:28 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1041 > https://issues.apache.org/jira/browse/MESOS-1041 > > > Repository: mesos-git > > > Description > ------- > > see summary > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a > 3rdparty/libprocess/include/process/check.hpp PRE-CREATION > 3rdparty/libprocess/include/process/collect.hpp > 2a73bc946fc4fa65229e9c979d841f2657a7cd2d > 3rdparty/libprocess/include/process/future.hpp > e45f4f79faeefbffc28d855d2f74e8df69099f18 > 3rdparty/libprocess/include/process/subprocess.hpp > 452eeeae0a9f2353db2d9302be594a578db52026 > > Diff: https://reviews.apache.org/r/18550/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
