[Xen-devel] [PATCH 2/3] libxl: event system: Make libxl_sigchld_owner_libxl_always work

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Previously, libxl_sigchld_owner_libxl_always was mishandled.  It would
result in libxl paying no attention to the sigchld self pipe.

Fix this by fixing chldmode_ours so that it returns true iff we are
supposed to be handling SIGCHLD.

Additionally, we arrange to use chldmode_ours everywhere where we are
installing/removing signal handlers and/or deciding whether to check
the self pipe, etc.  This means it needs a new "creating" flag
argument for the benefit of libxl__ev_child_fork, which needs to
install the signal handler in libxl_sigchld_owner_libxl even if there
are not currently any children.

ctx->childproc_hooks->chldowner is now interpreted only by
chldmode_ours.

Reported-by: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Cc: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
Cc: Jim Fehlig <jfehlig@xxxxxxxx>

---
v2: Get sense of chldmode test right in perhaps_removehandler (!)
---
 tools/libxl/libxl_fork.c |   51 ++++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index ae6184c..a581763 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -224,18 +224,23 @@ int libxl__sigchld_installhandler(libxl__gc *gc) /* 
non-reentrant */
     return rc;
 }
 
-static int chldmode_ours(libxl_ctx *ctx)
+static bool chldmode_ours(libxl_ctx *ctx, bool creating)
 {
-    return ctx->childproc_hooks->chldowner == libxl_sigchld_owner_libxl;
+    switch (ctx->childproc_hooks->chldowner) {
+    case libxl_sigchld_owner_libxl:
+        return creating || !LIBXL_LIST_EMPTY(&ctx->children);
+    case libxl_sigchld_owner_mainloop:
+        return 0;
+    case libxl_sigchld_owner_libxl_always:
+        return 1;
+    }
+    abort();
 }
 
 int libxl__fork_selfpipe_active(libxl_ctx *ctx)
 {
     /* Returns the fd to read, or -1 */
-    if (!chldmode_ours(ctx))
-        return -1;
-
-    if (LIBXL_LIST_EMPTY(&ctx->children))
+    if (!chldmode_ours(ctx, 0))
         return -1;
 
     return ctx->sigchld_selfpipe[0];
@@ -243,11 +248,21 @@ int libxl__fork_selfpipe_active(libxl_ctx *ctx)
 
 static void perhaps_removehandler(libxl__gc *gc)
 {
-    if (LIBXL_LIST_EMPTY(&CTX->children) &&
-        CTX->childproc_hooks->chldowner != libxl_sigchld_owner_libxl_always)
+    if (!chldmode_ours(CTX, 0))
         libxl__sigchld_removehandler(gc);
 }
 
+static int perhaps_installhandler(libxl__gc *gc, bool creating)
+{
+    int rc;
+
+    if (chldmode_ours(CTX, creating)) {
+        rc = libxl__sigchld_installhandler(gc);
+        if (rc) return rc;
+    }
+    return 0;
+}
+
 static int childproc_reaped(libxl__egc *egc, pid_t pid, int status)
 {
     EGC_GC;
@@ -286,7 +301,7 @@ void libxl__fork_selfpipe_woken(libxl__egc *egc)
      * ctx must be locked EXACTLY ONCE */
     EGC_GC;
 
-    while (chldmode_ours(CTX) /* in case the app changes the mode */) {
+    while (chldmode_ours(CTX, 0) /* in case the app changes the mode */) {
         int status;
         pid_t pid = waitpid(-1, &status, WNOHANG);
 
@@ -332,10 +347,7 @@ pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child 
*ch,
     CTX_LOCK;
     int rc;
 
-    if (chldmode_ours(CTX)) {
-        rc = libxl__sigchld_installhandler(gc);
-        if (rc) goto out;
-    }
+    perhaps_installhandler(gc, 1);
 
     pid_t pid =
         CTX->childproc_hooks->fork_replacement
@@ -382,17 +394,8 @@ void libxl_childproc_setmode(libxl_ctx *ctx, const 
libxl_childproc_hooks *hooks,
     ctx->childproc_hooks = hooks;
     ctx->childproc_user = user;
 
-    switch (ctx->childproc_hooks->chldowner) {
-    case libxl_sigchld_owner_mainloop:
-    case libxl_sigchld_owner_libxl:
-        libxl__sigchld_removehandler(gc);
-        break;
-    case libxl_sigchld_owner_libxl_always:
-        libxl__sigchld_installhandler(gc);
-        break;
-    default:
-        abort();
-    }
+    perhaps_removehandler(gc);
+    perhaps_installhandler(gc, 0); /* idempotent, ok to ignore errors for now 
*/
 
     CTX_UNLOCK;
     GC_FREE;
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel