xl: track child processes for the benefit of libxl

Each time xl forks, it needs to record the pid, so that its exit
status can be preserved if it happens that libxl's event loop reaped
it.  Consequently we also have a new wrapper for waitpid, which in
that case returns the previously-reaped status.

When we get a console ready callback from libxl, check to see if we
have spawned but not reaped a previous console client, and if so reap
it now.  (This is necessary to prevent improper use of the xlchild
struct, but has the happy consequence of checking the exit status of
the first console client in the pygrub case.)

Refactor the two calls to libxl_ctx_alloc into a new function
xl_ctx_alloc which also sets the child reaped handler callback.

All of this has the effect of suppressing a message
   unknown child [nnnn] unexpected exited status zero
which would sometimes (depending on a race) appear with `xl create -c'
and pygrub.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reported-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
[ ijc -- corrected return codes in xl_reaped_callback to match documented
         convention. ]
Committed-by: Ian Campbell <ian.campbell@citrix.com>

3 files changed, 136 insertions, 43 deletions

diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 750a61c54c..69f8737459 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -102,22 +102,85 @@ void postfork(void)
     libxl_postfork_child_noexec(ctx); /* in case we don't exit/exec */
     ctx = 0;
 
-    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, (xentoollog_logger*)logger)) {
-        fprintf(stderr, "cannot reinit xl context after fork\n");
-        exit(-1);
-    }
+    xl_ctx_alloc();
 }
 
-pid_t xl_fork(libxl_ctx *ctx) {
-    pid_t pid;
+pid_t xl_fork(xlchildnum child) {
+    xlchild *ch = &children[child];
+    int i;
+
+    assert(!ch->pid);
+    ch->reaped = 0;
 
-    pid = fork();
-    if (pid == -1) {
+    ch->pid = fork();
+    if (ch->pid == -1) {
         perror("fork failed");
         exit(-1);
     }
 
-    return pid;
+    if (!ch->pid) {
+        /* We are in the child now.  So all these children are not ours. */
+        for (i=0; i<child_max; i++)
+            children[i].pid = 0;
+    }
+
+    return ch->pid;
+}
+
+pid_t xl_waitpid(xlchildnum child, int *status, int flags)
+{
+    xlchild *ch = &children[child];
+    pid_t got = ch->pid;
+    assert(got);
+    if (ch->reaped) {
+        *status = ch->status;
+        ch->pid = 0;
+        return got;
+    }
+    for (;;) {
+        got = waitpid(ch->pid, status, flags);
+        if (got < 0 && errno == EINTR) continue;
+        if (got > 0) {
+            assert(got == ch->pid);
+            ch->pid = 0;
+        }
+        return got;
+    }
+}
+
+int xl_child_pid(xlchildnum child)
+{
+    xlchild *ch = &children[child];
+    return ch->pid;
+}
+
+static int xl_reaped_callback(pid_t got, int status, void *user)
+{
+    int i;
+    assert(got);
+    for (i=0; i<child_max; i++) {
+        xlchild *ch = &children[i];
+        if (ch->pid == got) {
+            ch->reaped = 1;
+            ch->status = status;
+            return 0;
+        }
+    }
+    return ERROR_UNKNOWN_CHILD;
+}
+
+static const libxl_childproc_hooks childproc_hooks = {
+    .chldowner = libxl_sigchld_owner_libxl,
+    .reaped_callback = xl_reaped_callback,
+};
+
+void xl_ctx_alloc(void) {
+    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, (xentoollog_logger*)logger)) {
+        fprintf(stderr, "cannot init xl context\n");
+        exit(1);
+    }
+
+    libxl_childproc_setmode(ctx, &childproc_hooks, 0);
 }
 
 int main(int argc, char **argv)
@@ -159,10 +222,7 @@ int main(int argc, char **argv)
     logger = xtl_createlogger_stdiostream(stderr, minmsglevel,  0);
     if (!logger) exit(1);
 
-    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, (xentoollog_logger*)logger)) {
-        fprintf(stderr, "cannot init xl context\n");
-        exit(1);
-    }
+    xl_ctx_alloc();
 
     /* Read global config file options */
     ret = asprintf(&config_file, "%s/xl.conf", libxl_xen_config_dir_path());
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index b7eacaa7e4..2af9428124 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -15,6 +15,8 @@
 #ifndef XL_H
 #define XL_H
 
+#include <assert.h>
+
 #include "xentoollog.h"
 
 struct cmd_spec {
@@ -108,8 +110,32 @@ struct cmd_spec *cmdtable_lookup(const char *s);
 
 extern libxl_ctx *ctx;
 extern xentoollog_logger_stdiostream *logger;
-pid_t xl_fork(libxl_ctx *ctx); /* like fork, but prints and dies if it fails */
-void postfork(void);
+
+void xl_ctx_alloc(void);
+
+/* child processes */
+
+typedef struct {
+    /* every struct like this must be in XLCHILD_LIST */
+    pid_t pid; /* 0: not in use */
+    int reaped; /* valid iff pid!=0 */
+    int status; /* valid iff reaped */
+} xlchild;
+
+typedef enum {
+    child_console, child_waitdaemon, child_migration,
+    child_max
+} xlchildnum;
+
+extern xlchild children[child_max];
+
+pid_t xl_fork(xlchildnum); /* like fork, but prints and dies if it fails */
+void postfork(void); /* needed only if we aren't going to exec right away */
+
+/* Handles EINTR.  Clears out the xlchild so it can be reused. */
+pid_t xl_waitpid(xlchildnum, int *status, int flags);
+
+int xl_child_pid(xlchildnum); /* returns 0 if child struct is not in use */
 
 /* global options */
 extern int autoballoon;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c55a693a7..dbe5633c74 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -17,7 +17,6 @@
 #include "libxl_osdeps.h"
 
 #include <stdio.h>
-#include <assert.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -66,6 +65,8 @@ int logfile = 2;
 /* every libxl action in xl uses this same libxl context */
 libxl_ctx *ctx;
 
+xlchild children[child_max];
+
 /* when we operate on a domain, it is this one: */
 static uint32_t domid;
 static const char *common_domname;
@@ -1496,19 +1497,33 @@ static int freemem(libxl_domain_build_info *b_info)
     return ERROR_NOMEM;
 }
 
+static void console_child_report(void)
+{
+    if (xl_child_pid(child_console)) {
+        int status;
+        pid_t got = xl_waitpid(child_console, &status, 0);
+        if (got < 0)
+            perror("xl: warning, failed to waitpid for console child");
+        else if (status)
+            libxl_report_child_exitstatus(ctx, XTL_ERROR, "console child",
+                                          xl_child_pid(child_console), status);
+    }
+}
+
 static void autoconnect_console(libxl_ctx *ctx_ignored,
                                 libxl_event *ev, void *priv)
 {
-    pid_t *pid = priv;
     uint32_t bldomid = ev->domid;
 
     libxl_event_free(ctx, ev);
 
-    *pid = fork();
-    if (*pid < 0) {
+    console_child_report();
+
+    pid_t pid = xl_fork(child_console);
+    if (pid < 0) {
         perror("unable to fork xenconsole");
         return;
-    } else if (*pid > 0)
+    } else if (pid > 0)
         return;
 
     postfork();
@@ -1580,7 +1595,6 @@ static int create_domain(struct domain_create *dom_info)
     int restore_fd = -1;
     int status = 0;
     const libxl_asyncprogress_how *autoconnect_console_how;
-    pid_t child_console_pid = -1;
     struct save_file_header hdr;
 
     int restoring = (restore_file || (migrate_fd >= 0));
@@ -1741,7 +1755,6 @@ start:
     libxl_asyncprogress_how autoconnect_console_how_buf;
     if ( dom_info->console_autoconnect ) {
         autoconnect_console_how_buf.callback = autoconnect_console;
-        autoconnect_console_how_buf.for_callback = &child_console_pid;
         autoconnect_console_how = &autoconnect_console_how_buf;
     }else{
         autoconnect_console_how = 0;
@@ -1813,19 +1826,17 @@ start:
         pid_t child1, got_child;
         int nullfd;
 
-        child1 = xl_fork(ctx);
+        child1 = xl_fork(child_waitdaemon);
         if (child1) {
             printf("Daemon running with PID %d\n", child1);
 
             for (;;) {
-                got_child = waitpid(child1, &status, 0);
+                got_child = xl_waitpid(child_waitdaemon, &status, 0);
                 if (got_child == child1) break;
                 assert(got_child == -1);
-                if (errno != EINTR) {
-                    perror("failed to wait for daemonizing child");
-                    ret = ERROR_FAIL;
-                    goto out;
-                }
+                perror("failed to wait for daemonizing child");
+                ret = ERROR_FAIL;
+                goto out;
             }
             if (status) {
                 libxl_report_child_exitstatus(ctx, XTL_ERROR,
@@ -1986,10 +1997,7 @@ out:
 
     free(config_data);
 
-waitpid_out:
-    if (child_console_pid > 0 &&
-            waitpid(child_console_pid, &status, 0) < 0 && errno == EINTR)
-        goto waitpid_out;
+    console_child_report();
 
     if (deathw)
         libxl_evdisable_domain_death(ctx, deathw);
@@ -2833,7 +2841,7 @@ static pid_t create_migration_child(const char *rune, int *send_fd,
     MUST( libxl_pipe(ctx, sendpipe) );
     MUST( libxl_pipe(ctx, recvpipe) );
 
-    child = xl_fork(ctx);
+    child = xl_fork(child_migration);
 
     if (!child) {
         dup2(sendpipe[0], 0);
@@ -2877,19 +2885,20 @@ static int migrate_read_fixedmessage(int fd, const void *msg, int msgsz,
     return 0;
 }
 
-static void migration_child_report(pid_t migration_child, int recv_fd) {
+static void migration_child_report(int recv_fd) {
     pid_t child;
     int status, sr;
     struct timeval now, waituntil, timeout;
     static const struct timeval pollinterval = { 0, 1000 }; /* 1ms */
 
-    if (!migration_child) return;
+    if (!xl_child_pid(child_migration)) return;
 
     CHK_ERRNO( gettimeofday(&waituntil, 0) );
     waituntil.tv_sec += 2;
 
     for (;;) {
-        child = waitpid(migration_child, &status, WNOHANG);
+        pid_t migration_child = xl_child_pid(child_migration);
+        child = xl_waitpid(child_migration, &status, WNOHANG);
 
         if (child == migration_child) {
             if (status)
@@ -2899,7 +2908,6 @@ static void migration_child_report(pid_t migration_child, int recv_fd) {
             break;
         }
         if (child == -1) {
-            if (errno == EINTR) continue;
             fprintf(stderr, "wait for migration child [%ld] failed: %s\n",
                     (long)migration_child, strerror(errno));
             break;
@@ -2939,7 +2947,6 @@ static void migration_child_report(pid_t migration_child, int recv_fd) {
             }
         }
     }
-    migration_child = 0;
 }
 
 static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
@@ -2958,7 +2965,7 @@ static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
                                    "banner", rune);
     if (rc) {
         close(send_fd);
-        migration_child_report(child, recv_fd);
+        migration_child_report(recv_fd);
         exit(-rc);
     }
 
@@ -3083,13 +3090,13 @@ static void migrate_domain(const char *domain_spec, const char *rune,
 
  failed_suspend:
     close(send_fd);
-    migration_child_report(child, recv_fd);
+    migration_child_report(recv_fd);
     fprintf(stderr, "Migration failed, failed to suspend at sender.\n");
     exit(-ERROR_FAIL);
 
  failed_resume:
     close(send_fd);
-    migration_child_report(child, recv_fd);
+    migration_child_report(recv_fd);
     fprintf(stderr, "Migration failed, resuming at sender.\n");
     libxl_domain_resume(ctx, domid, 0);
     exit(-ERROR_FAIL);
@@ -3104,7 +3111,7 @@ static void migrate_domain(const char *domain_spec, const char *rune,
  " responsibility to avoid that.  Sorry.\n");
 
     close(send_fd);
-    migration_child_report(child, recv_fd);
+    migration_child_report(recv_fd);
     exit(-ERROR_BADFAIL);
 }