libxl: child processes cleanups

Abolish libxl_fork.  Its only callers were in xl.  Its functionality
is now moved elsewhere, as follows:

* The "logging version of fork", which is what it was billed as, is now
  xl_fork, which also dies on failure.

* Closing the xenstore handle in the child is now done in
  libxl__ev_child_fork, which is the only remaining place where fork
  is called in libxl.

* We provide a new function libxl__ev_child_xenstore_reopen for
  in-libxl children to make the ctx useable for xenstore again.

* Consequently libxl__spawn_record_pid now no longer needs to mess
  about with its own xenstore handle.  As a bonus it can now just use
  libxl__xs_write.

Also, since we have now converted all the forkers in libxl, we can
always honour the fork_replacement childproc hook - so do so.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Committed-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

8 files changed, 61 insertions, 46 deletions

diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 882c048faf..b46b8939ae 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -127,34 +127,23 @@ void libxl_report_child_exitstatus(libxl_ctx *ctx,
     }
 }
 
-int libxl__spawn_record_pid(libxl__gc *gc, libxl__spawn_state *spawn,
-                            pid_t innerchild)
+int libxl__spawn_record_pid(libxl__gc *gc, libxl__spawn_state *spawn, pid_t pid)
 {
-    struct xs_handle *xsh = NULL;
-    const char *pid = NULL;
-    int rc, xsok;
+    int r, rc;
 
-    pid = GCSPRINTF("%d", innerchild);
+    rc = libxl__ev_child_xenstore_reopen(gc, spawn->what);
+    if (rc) goto out;
 
-    /* we mustn't use the parent's handle in the child */
-    xsh = xs_daemon_open();
-    if (!xsh) {
-        LOGE(ERROR, "write %s = %s: xenstore reopen failed",
-             spawn->pidpath, pid);
-        rc = ERROR_FAIL;  goto out;
-    }
-
-    xsok = xs_write(xsh, XBT_NULL, spawn->pidpath, pid, strlen(pid));
-    if (!xsok) {
+    r = libxl__xs_write(gc, XBT_NULL, spawn->pidpath, "%d", pid);
+    if (r) {
         LOGE(ERROR,
-             "write %s = %s: xenstore write failed", spawn->pidpath, pid);
+             "write %s = %d: xenstore write failed", spawn->pidpath, pid);
         rc = ERROR_FAIL;  goto out;
     }
 
     rc = 0;
 
 out:
-    if (xsh) xs_daemon_close(xsh);
     return rc ? SIGTERM : 0;
 }
 
@@ -302,7 +291,15 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
 
     /* we are now the middle process */
 
-    child = fork();
+    pid_t (*fork_replacement)(void*) =
+        CTX->childproc_hooks
+        ? CTX->childproc_hooks->fork_replacement
+        : 0;
+    child =
+        fork_replacement
+        ? fork_replacement(CTX->childproc_user)
+        : fork();
+
     if (child == -1)
         exit(255);
     if (!child) {
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 35c8bdd7f3..9ff99e0bfb 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -347,7 +347,12 @@ pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *ch,
 
     if (!pid) {
         /* woohoo! */
-        return 0; /* Yes, CTX is left locked in the child. */
+        if (CTX->xsh) {
+            xs_daemon_destroy_postfork(CTX->xsh);
+            CTX->xsh = NULL; /* turns mistakes into crashes */
+        }
+        /* Yes, CTX is left locked in the child. */
+        return 0;
     }
 
     ch->pid = pid;
@@ -395,6 +400,24 @@ const libxl_childproc_hooks libxl__childproc_default_hooks = {
     libxl_sigchld_owner_libxl, 0, 0
 };
 
+int libxl__ev_child_xenstore_reopen(libxl__gc *gc, const char *what) {
+    int rc;
+
+    assert(!CTX->xsh);
+    CTX->xsh = xs_daemon_open();
+    if (!CTX->xsh) {
+        LOGE(ERROR, "%s: xenstore reopen failed", what);
+        rc = ERROR_FAIL;  goto out;
+    }
+
+    libxl_fd_set_cloexec(CTX, xs_fileno(CTX->xsh), 1);
+
+    return 0;
+
+ out:
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9f3f759fea..83fabea6ef 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -640,6 +640,11 @@ static inline void libxl__ev_child_init(libxl__ev_child *childw_out)
 static inline int libxl__ev_child_inuse(libxl__ev_child *childw_out)
                 { return childw_out->pid >= 0; }
 
+/* Useable (only) in the child to once more make the ctx useable for
+ * xenstore operations.  logs failure in the form "what: <error
+ * message>". */
+_hidden int libxl__ev_child_xenstore_reopen(libxl__gc *gc, const char *what);
+
 
 /*
  * Other event-handling support provided by the libxl event core to
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index f0d94c6828..858410e7bc 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -444,27 +444,6 @@ int libxl__remove_directory(libxl__gc *gc, const char *dirpath)
     return rc;
 }
 
-pid_t libxl_fork(libxl_ctx *ctx)
-{
-    pid_t pid;
-
-    pid = fork();
-    if (pid == -1) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fork failed");
-        return -1;
-    }
-
-    if (!pid) {
-        if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh);
-        ctx->xsh = 0;
-        /* This ensures that anyone who forks but doesn't exec,
-         * and doesn't reinitialise the libxl_ctx, is OK.
-         * It also means they can safely call libxl_ctx_free. */
-    }
-
-    return pid;
-}
-
 int libxl_pipe(libxl_ctx *ctx, int pipes[2])
 {
     if (pipe(pipes) < 0) {
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 2b47622171..74beb52a67 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -47,9 +47,8 @@ int libxl_write_exactly(libxl_ctx *ctx, int fd, const void *data,
    * logged using filename (which is only used for logging) and what
    * (which may be 0). */
 
-pid_t libxl_fork(libxl_ctx *ctx);
 int libxl_pipe(libxl_ctx *ctx, int pipes[2]);
-  /* Just like fork(2), pipe(2), but log errors. */
+  /* Just like pipe(2), but log errors. */
 
 void libxl_report_child_exitstatus(libxl_ctx *ctx, xentoollog_level,
                                    const char *what, pid_t pid, int status);
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index a6ffd25ff7..d4db1f8b8b 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -105,6 +105,18 @@ void postfork(void)
     }
 }
 
+pid_t xl_fork(libxl_ctx *ctx) {
+    pid_t pid;
+
+    pid = fork();
+    if (pid == -1) {
+        perror("fork failed");
+        exit(-1);
+    }
+
+    return pid;
+}
+
 int main(int argc, char **argv)
 {
     int opt = 0;
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index f0d0ec8870..2b6714af36 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -104,6 +104,7 @@ 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);
 
 /* global options */
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b91a2d5911..efaf3de047 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1736,7 +1736,7 @@ start:
         pid_t child1, got_child;
         int nullfd;
 
-        child1 = libxl_fork(ctx);
+        child1 = xl_fork(ctx);
         if (child1) {
             printf("Daemon running with PID %d\n", child1);
 
@@ -2779,8 +2779,7 @@ static void migrate_domain(const char *domain_spec, const char *rune,
     MUST( libxl_pipe(ctx, sendpipe) );
     MUST( libxl_pipe(ctx, recvpipe) );
 
-    child = libxl_fork(ctx);
-    if (child==-1) exit(1);
+    child = xl_fork(ctx);
 
     if (!child) {
         dup2(sendpipe[0], 0);