> On Oct. 12, 2018, 5:40 p.m., James Peach wrote: > > 3rdparty/libprocess/src/posix/subprocess.hpp > > Lines 213 (patched) > > <https://reviews.apache.org/r/68644/diff/2/?file=2096378#file2096378line213> > > > > Since you are planning a different code path for macOS, maybe hoist > > this out into a static support function in preparation? > > Qian Zhang wrote: > Did you mean we should put those codes into a static function like > `static int convertStringToInt(const char *name)`?
Yes, exactly. > On Oct. 12, 2018, 5:40 p.m., James Peach wrote: > > 3rdparty/libprocess/src/posix/subprocess.hpp > > Lines 255 (patched) > > <https://reviews.apache.org/r/68644/diff/2/?file=2096378#file2096378line255> > > > > You can just use `std::find()` here. > > Qian Zhang wrote: > That's what I was thinking. But it seems `std::find()` may allocate > memory (search `allocate memory` in > https://en.cppreference.com/w/cpp/algorithm/find )? Oh, I guess that it's not guaranteed to not allocate. Let's drop this then. > On Oct. 12, 2018, 5:40 p.m., James Peach wrote: > > 3rdparty/libprocess/src/posix/subprocess.hpp > > Lines 275 (patched) > > <https://reviews.apache.org/r/68644/diff/2/?file=2096378#file2096378line275> > > > > Unfortunately, the `Try` here is not async-signal-safe. However, that > > is already used by `UNSET_CLOEXEC`, so I think we can just leave a TODO > > here. > > > > Can you file a JIRA to add something like `signal_save::uncloexec()`? > > Qian Zhang wrote: > I see we also use `Try` in another place in `childMain`, e.g., > `Try<Nothing> callback = hook();`, so that one needs to be changed too? > > And I'd like to call `fcntl` here directly to unset the `close-on-exec` > flag. > I see we also use Try in another place in childMain, e.g., Try<Nothing> > callback = hook();, so that one needs to be changed too? Yes, in principle. I don't think we need to address that here though. > And I'd like to call fcntl here directly to unset the close-on-exec flag. That seems fine to me. Previously, we put helpers in the `signal_safe` namespace, but having a local helper for this case seems OK too. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68644/#review209489 ----------------------------------------------------------- On Oct. 14, 2018, 2:05 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68644/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2018, 2:05 p.m.) > > > Review request for mesos, Gilbert Song and James Peach. > > > Bugs: MESOS-9152 and MESOS-9164 > https://issues.apache.org/jira/browse/MESOS-9152 > https://issues.apache.org/jira/browse/MESOS-9164 > > > Repository: mesos > > > Description > ------- > > Closed all file descriptors except `whitelist_fds` in posix/subprocess. > > > Diffs > ----- > > 3rdparty/libprocess/src/posix/subprocess.hpp > 007058b61fdcd4716aa793516c842c3cef8c0a29 > 3rdparty/libprocess/src/subprocess.cpp > c0640de2dc4278b884282dfaad98c49c3b067a5b > > > Diff: https://reviews.apache.org/r/68644/diff/3/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
