libxl: event system: properly register the SIGCHLD self-pipe

An application which uses libxl_osevent_register_hooks, and doesn't
use libxl_sigchld_owner_mainloop, would never properly experience the
deaths of its (libxl) children.

This is because the SIGCHLD self pipe would be handled using ad-hoc
code in beforepoll_internal and afterpoll_internal.  However, this is
no good if application is using the fd registration system instead; in
that case these functions would not be called and nothing would deal
with readability of the self pipe.

Fix this as follows:

The SIGCHLD self pipe now is now dealt with by a standard libxl__ev_fd
handler, which is registered and deregistered along with the SIGCHLD
handler itself.  The handler function subsumes the ad-hoc response
code removed from beforepoll_internal, and the functionality of
libxl__fork_selfpipe_woken.

This is also tidier as the SIGCHLD self pipe is no longer touched
outside libxl_fork.c other than in ctx initialisation and teardown.

(The ad-hoc arrangements for poller->wakeup_pipe in
beforepoll_internal and afterpoll_internal are OK because the
libxl__poller mechanism exists to wake up threads which are sitting
inside libxl's poll loop, so is not applicable to the application's
event loop.)

Reported-by: Bamvor Jian Zhang <bjzhang@suse.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b72e3a79d6555313a770b356fd6462ee3bf44cf9..9b932628e1701bf736f55602faf097fcfa32785f 100644 (file)
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -64,6 +64,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     ctx->childproc_user = 0;
         
     ctx->sigchld_selfpipe[0] = -1;
+    libxl__ev_fd_init(&ctx->sigchld_selfpipe_efd);
 
     /* The mutex is special because we can't idempotently destroy it */
 
@@ -139,6 +140,7 @@ int libxl_ctx_free(libxl_ctx *ctx)
     for (i = 0; i < ctx->watch_nslots; i++)
         assert(!libxl__watch_slot_contents(gc, i));
     libxl__ev_fd_deregister(gc, &ctx->watch_efd);
+    libxl__ev_fd_deregister(gc, &ctx->sigchld_selfpipe_efd);
 
     /* Now there should be no more events requested from the application: */
 

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 54d15dbbfadd7ec0623fcdc0fe0779c6d3a34456..bdef7acdb46e2c98a35c196002c4f287b7b74c16 100644 (file)
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -774,10 +774,6 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
                                                                        \
         REQUIRE_FD(poller->wakeup_pipe[0], POLLIN, BODY);              \
                                                                        \
-        int selfpipe = libxl__fork_selfpipe_active(CTX);               \
-        if (selfpipe >= 0)                                             \
-            REQUIRE_FD(selfpipe, POLLIN, BODY);                        \
-                                                                       \
     }while(0)
 
 #define REQUIRE_FD(req_fd_, req_events_, BODY) do{      \
@@ -999,14 +995,6 @@ static void afterpoll_internal(libxl__egc *egc, libxl__poller *poller,
         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)) {
-        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)

diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index a5817635825832d932cca632dbbb4be55ef78d04..4ae9f94836fcb3c9429d628b802bcc09c3946b2f 100644 (file)
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -155,6 +155,9 @@ int libxl__carefd_fd(const libxl__carefd *cf)
  * Actual child process handling
  */
 
+static void sigchld_selfpipe_handler(libxl__egc *egc, libxl__ev_fd *ev,
+                                     int fd, short events, short revents);
+
 static void sigchld_handler(int signo)
 {
     int esave = errno;
@@ -177,10 +180,18 @@ static void sigchld_removehandler_core(void)
 
 void libxl__sigchld_removehandler(libxl__gc *gc) /* non-reentrant */
 {
+    int rc;
+
     atfork_lock();
     if (sigchld_owner == CTX)
         sigchld_removehandler_core();
     atfork_unlock();
+
+    if (libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) {
+        rc = libxl__ev_fd_modify(gc, &CTX->sigchld_selfpipe_efd, 0);
+        if (rc)
+            libxl__ev_fd_deregister(gc, &CTX->sigchld_selfpipe_efd);
+    }
 }
 
 int libxl__sigchld_installhandler(libxl__gc *gc) /* non-reentrant */
@@ -197,6 +208,15 @@ int libxl__sigchld_installhandler(libxl__gc *gc) /* non-reentrant */
             goto out;
         }
     }
+    if (!libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) {
+        rc = libxl__ev_fd_register(gc, &CTX->sigchld_selfpipe_efd,
+                                   sigchld_selfpipe_handler,
+                                   CTX->sigchld_selfpipe[0], POLLIN);
+        if (rc) goto out;
+    } else {
+        rc = libxl__ev_fd_modify(gc, &CTX->sigchld_selfpipe_efd, POLLIN);
+        if (rc) goto out;
+    }
 
     atfork_lock();
     if (sigchld_owner != CTX) {
@@ -237,15 +257,6 @@ static bool chldmode_ours(libxl_ctx *ctx, bool creating)
     abort();
 }
 
-int libxl__fork_selfpipe_active(libxl_ctx *ctx)
-{
-    /* Returns the fd to read, or -1 */
-    if (!chldmode_ours(ctx, 0))
-        return -1;
-
-    return ctx->sigchld_selfpipe[0];
-}
-
 static void perhaps_removehandler(libxl__gc *gc)
 {
     if (!chldmode_ours(CTX, 0))
@@ -295,12 +306,29 @@ int libxl_childproc_reaped(libxl_ctx *ctx, pid_t pid, int status)
     return rc;
 }
 
-void libxl__fork_selfpipe_woken(libxl__egc *egc)
+static void sigchld_selfpipe_handler(libxl__egc *egc, libxl__ev_fd *ev,
+                                     int fd, short events, short revents)
 {
     /* May make callbacks into the application for child processes.
-     * ctx must be locked EXACTLY ONCE */
+     * So, this function may unlock and relock the CTX.  This is OK
+     * because event callback functions are always called with the CTX
+     * locked exactly once, and from code which copes with reentrancy.
+     * (See also the comment in afterpoll_internal.) */
     EGC_GC;
 
+    int selfpipe = CTX->sigchld_selfpipe[0];
+
+    if (revents & ~POLLIN) {
+        LOG(ERROR, "unexpected poll event 0x%x on SIGCHLD self pipe", revents);
+        LIBXL__EVENT_DISASTER(egc,
+                              "unexpected poll event on SIGCHLD self pipe",
+                              0, 0);
+    }
+    assert(revents & POLLIN);
+
+    int e = libxl__self_pipe_eatall(selfpipe);
+    if (e) LIBXL__EVENT_DISASTER(egc, "read sigchld pipe", e, 0);
+
     while (chldmode_ours(CTX, 0) /* in case the app changes the mode */) {
         int status;
         pid_t pid = waitpid(-1, &status, WNOHANG);

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7310dc9377896927f38483735063274708ccfbbb..a2d8247f38b3aa83caee3b3f879fd5d3fe57555a 100644 (file)
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -353,6 +353,7 @@ struct libxl__ctx {
     const libxl_childproc_hooks *childproc_hooks;
     void *childproc_user;
     int sigchld_selfpipe[2]; /* [0]==-1 means handler not installed */
+    libxl__ev_fd sigchld_selfpipe_efd;
     LIBXL_LIST_HEAD(, libxl__ev_child) children;
 
     libxl_version_info version_info;
@@ -839,8 +840,6 @@ _hidden void libxl__poller_wakeup(libxl__egc *egc, libxl__poller *p);
 extern const libxl_childproc_hooks libxl__childproc_default_hooks;
 int libxl__sigchld_installhandler(libxl__gc*); /* non-reentrant; logs errs */
 void libxl__sigchld_removehandler(libxl__gc*); /* non-reentrant */
-int libxl__fork_selfpipe_active(libxl_ctx *ctx); /* returns read fd or -1 */
-void libxl__fork_selfpipe_woken(libxl__egc *egc);
 int libxl__self_pipe_wakeup(int fd); /* returns 0 or -1 setting errno */
 int libxl__self_pipe_eatall(int fd); /* returns 0 or -1 setting errno */