> On Sept. 6, 2018, 10:47 p.m., Benjamin Mahler wrote: > > 3rdparty/libprocess/src/posix/libev/libev.cpp > > Lines 84-89 (patched) > > <https://reviews.apache.org/r/68660/diff/1/?file=2086293#file2086293line84> > > > > * Let's use sigaction here rather than ::signal? See the portability > > notes here: http://man7.org/linux/man-pages/man2/signal.2.html > > > > * Why do you need to set SIG_DFL? Can we just retrieve the old value > > and restore it later (note that sigaction makes this possible). > > > > * Where's the race here? Can you clarify? > > > > * Let's also remove the libev patch file if we have this proper > > approach to disabling it? > > > > * Note that we have some stuff in os/signals.hpp, I don't know if you > > want to extend that stuff or not, either way seems fine to me.
> Let's use sigaction here rather than ::signal? See the portability notes > here: http://man7.org/linux/man-pages/man2/signal.2.html > Why do you need to set SIG_DFL? Can we just retrieve the old value and > restore it later (note that sigaction makes this possible). Setting `SIG_DFL` is not important, I'm just trying to sample the old handler value. You are right about using `sigaction` though, that's a better approach. > Where's the race here? Can you clarify? Just generically mentioning that manipulating the signal handling is racey. > Let's also remove the libev patch file if we have this proper approach to > disabling it? The patch causes the whole feature to be compiled out of libev, so it still results in a benefit (smaller code). > Note that we have some stuff in os/signals.hpp, I don't know if you want to > extend that stuff or not, either way seems fine to me. I'll probably use `sigaction` directly. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68660/#review208427 ----------------------------------------------------------- On Sept. 6, 2018, 7:19 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68660/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2018, 7:19 p.m.) > > > Review request for mesos, Benjamin Bannier, Till Toenshoff, and Vinod Kone. > > > Bugs: MESOS-9212 > https://issues.apache.org/jira/browse/MESOS-9212 > > > Repository: mesos > > > Description > ------- > > libev will consume SIGCHLD signals by default, which interferes > with a number of libprocess assumptions around dealing with child > processes. The recommended way to disable this behavior is to reset > the SIGCHLD signal handler after initializing the libeve default > event loop. > > > Diffs > ----- > > 3rdparty/libprocess/src/posix/libev/libev.cpp > 173ee466e61d3a754202213b94645e125ce0421a > > > Diff: https://reviews.apache.org/r/68660/diff/1/ > > > Testing > ------- > > configure with --with-libev=/usr, make check (Fedora 28) > > > Thanks, > > James Peach > >
