> 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? > > Benjamin Hindman wrote: > This doesn't look resolved? > > Dominic Hamon wrote: > i had a CHECK_READY, but that introduced a circular dependency between > check.hpp and future.hpp. > > I'll put back the full version (unless you can think of a way to avoid > the dependency).
Aha, bummer. Yeah, let's revert but also a TODO to replace after we move the definitions from check into a .cpp file. Thanks! > 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? > > Benjamin Hindman wrote: > 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? > > Dominic Hamon wrote: > i can just extend the existing abort in 18549.. it seems a reasonable > extension. Okay, I'll hold off on committing that one for a second. ;) - 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 > >
