Re: [PATCH] libxl: check pending child signals on libxl__ev_child_inuse

[Roger Pau Monne <roger.pau@citrix.com>]

Roger Pau Monne wrote:
> Since we might call libxl__ev_child_inuse from the callback of another
> event, and we might have pending signals from dead processes, update
> the processes status before returning to the caller.
>
> This was noticed because we processed POLLHUP before the child pipe,
> so we ended up trying to kill the child process from the PULLHUP
> callback, when the child was already dead. With this patch
> libxl__ev_child_inuse returns false, and we no longer try to kill an
> already dead child.

This is not correct, since we might assert that the child is running 
after a fork, but if the execution time is very short it might be dead.

A better solution might be to change the order in which events are 
processed, the following patch makes libxl process child events before 
anything else.

8<--------------------------------------------------------------------

[PATCH] libxl: check child signals before other events

Check pending signals from child processes before processing other
events, since the callbacks for other events might check the status of
the child and try to kill them, when they might already be dead.

Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
  tools/libxl/libxl_event.c |   30 +++++++++++++++++-------------
  1 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 4b02cd7..fe1cf49 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -916,6 +916,23 @@ static void afterpoll_internal(libxl__egc *egc, 
libxl__poller *poller,
       *   CTX->efds    is more complicated; see below.
       */

+    /*
+     * We should check for child signals before doing anything else,
+     * since callbacks for other events might check the child status.
+     */
+    if (afterpoll_check_fd(poller,fds,nfds, 
poller->wakeup_pipe[0],POLLIN, 0)) {
+        int e = libxl__self_pipe_eatall(poller->wakeup_pipe[0]);
+        if (e) LIBXL__EVENT_DISASTER(egc, "read wakeup", e, 0);
+    }
+
+    int selfpipe = libxl__fork_selfpipe_active(CTX);
+    if (selfpipe >= 0 &&
+        afterpoll_check_fd(poller,fds,nfds, selfpipe, POLLIN, 0)) {
+        int e = libxl__self_pipe_eatall(selfpipe);
+        if (e) LIBXL__EVENT_DISASTER(egc, "read sigchld pipe", e, 0);
+        libxl__fork_selfpipe_woken(egc);
+    }
+
      for (;;) {
          /* We restart our scan of fd events whenever we call a
           * callback function.  This is necessary because such
@@ -949,19 +966,6 @@ static void afterpoll_internal(libxl__egc *egc, 
libxl__poller *poller,
                  *(rindices[i]) = INT_MAX;
      }

-    if (afterpoll_check_fd(poller,fds,nfds, 
poller->wakeup_pipe[0],POLLIN, 0)) {
-        int e = libxl__self_pipe_eatall(poller->wakeup_pipe[0]);
-        if (e) LIBXL__EVENT_DISASTER(egc, "read wakeup", e, 0);
-    }
-
-    int selfpipe = libxl__fork_selfpipe_active(CTX);
-    if (selfpipe >= 0 &&
-        afterpoll_check_fd(poller,fds,nfds, selfpipe, POLLIN, 0)) {
-        int e = libxl__self_pipe_eatall(selfpipe);
-        if (e) LIBXL__EVENT_DISASTER(egc, "read sigchld pipe", e, 0);
-        libxl__fork_selfpipe_woken(egc);
-    }
-
      for (;;) {
          libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
          if (!etime)
-- 
1.7.7.5 (Apple Git-26)