----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51277/#review146454 -----------------------------------------------------------
Fix it, then Ship it! LGTM! src/linux/ns.hpp (line 283) <https://reviews.apache.org/r/51277/#comment212925> I'd add a comment saying that `sockets[0]` is the read end and `sockets[1]` is the write end. Unlike `pipe`, socketpair is bidirectional. Therefore, comment is important. src/linux/ns.hpp (line 397) <https://reviews.apache.org/r/51277/#comment212940> Use assert instead. CHECK will initialize glog, which is not async safe. src/linux/ns.hpp (line 399) <https://reviews.apache.org/r/51277/#comment212941> streaming functions are definitey not async safe. Also `strerror` is not safe as well http://austin-group-l.opengroup.narkive.com/jBp07fPN/adding-simple-string-functions-to-async-signal-safe-list You forgot to remove it after debugging? We should not add side effect to this helper function. I'd suggest we simply remove it. i think the only reason setns can fail here is EPERM. Maybe you should add a comment saything that this function should only be invoked when the caller is privileged. src/linux/ns.hpp (line 402) <https://reviews.apache.org/r/51277/#comment212944> `exit` is not async safe because it'll invoke destructors. use `_exit` instead src/linux/ns.hpp (line 411) <https://reviews.apache.org/r/51277/#comment212943> Can you add some optimizaiton here to avoid the extra fork if `fds.get(CLONE_NEWPID).isNone() || fds.get(CLONE_NEWUSER).isNone()`? At least add a TODO? src/linux/ns.hpp (line 422) <https://reviews.apache.org/r/51277/#comment212945> Ditto on using `_exit` src/linux/ns.hpp (line 430) <https://reviews.apache.org/r/51277/#comment212895> os::clone is not async signal safe. It does an dynamic memory allocation on the heap. We may want to allow os::clone to take a pointer to the stack. In ns::enter, we can pre-allocate the stack on the heap. src/linux/ns.hpp (lines 434 - 435) <https://reviews.apache.org/r/51277/#comment212947> I don't get this comment. f() below will eventually exec the user command, right? What do you mean by `exit no matter what`. src/linux/ns.hpp (line 452) <https://reviews.apache.org/r/51277/#comment212946> Ditto on using `_exit` - Jie Yu On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51277/ > ----------------------------------------------------------- > > (Updated Aug. 22, 2016, 4:54 a.m.) > > > Review request for mesos and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Added an 'ns::enter' helper. > > > Diffs > ----- > > src/linux/ns.hpp 2d6c359ed24d6e964882f34df60d8182491a27c9 > > Diff: https://reviews.apache.org/r/51277/diff/ > > > Testing > ------- > > > Thanks, > > Benjamin Hindman > >
