[libvirt] [PATCHv2 3/3] command: add virCommandAbort for cleanup paths

[Eric Blake]

Sometimes, an asynchronous helper is started (such as a compressor
or iohelper program), but a later error means that we want to
abort that child.  Make this easier.

Note that since daemons and virCommandRunAsync can't mix, the only
time virCommandFree can reap a process is if someone did
virCommandRunAsync for a non-daemon and didn't stash the pid.

* src/util/command.h (virCommandAbort): New prototype.
* src/util/command.c (_virCommand): Add new field.
(virCommandRunAsync, virCommandWait): Track whether pid was used.
(virCommandFree): Reap child if caller did not request pid.
(virCommandAbort): New function.
* src/libvirt_private.syms (command.h): Export it.
* tests/commandtest.c (test19): New test.
---

v2: no change, but by adding patch 2, it should make it clear
that this patch is doing the right thing about not reaping
a long-running daemon.

 src/libvirt_private.syms |    1 +
 src/util/command.c       |   66 ++++++++++++++++++++++++++++++++++++++++++++++
 src/util/command.h       |   12 +++++++-
 tests/commandtest.c      |   37 +++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 1 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f783c63..5f58970 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -91,6 +91,7 @@ virCgroupSetMemSwapHardLimit;


 # command.h
+virCommandAbort;
 virCommandAddArg;
 virCommandAddArgBuffer;
 virCommandAddArgFormat;
diff --git a/src/util/command.c b/src/util/command.c
index 3a8ffae..905256e 100644
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -81,6 +81,7 @@ struct _virCommand {

     pid_t pid;
     char *pidfile;
+    bool reap;
 };

 /*
@@ -1222,6 +1223,8 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)

     if (ret == 0 && pid)
         *pid = cmd->pid;
+    else
+        cmd->reap = true;

     return ret;
 }
@@ -1267,6 +1270,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
     }

     cmd->pid = -1;
+    cmd->reap = false;

     if (exitstatus == NULL) {
         if (status != 0) {
@@ -1288,6 +1292,65 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)


 /*
+ * Abort an async command if it is running, without issuing
+ * any errors or affecting errno.  Designed for error paths
+ * where some but not all paths to the cleanup code might
+ * have started the child process.
+ */
+void
+virCommandAbort(virCommandPtr cmd)
+{
+    int saved_errno;
+    int ret;
+    int status;
+    char *tmp = NULL;
+
+    if (!cmd || cmd->pid == -1)
+        return;
+
+    /* See if intermediate process has exited; if not, try a nice
+     * SIGTERM followed by a more severe SIGKILL.
+     */
+    saved_errno = errno;
+    VIR_DEBUG("aborting child process %d", cmd->pid);
+    while ((ret = waitpid(cmd->pid, &status, WNOHANG)) == -1 &&
+           errno == EINTR);
+    if (ret == cmd->pid) {
+        tmp = virCommandTranslateStatus(status);
+        VIR_DEBUG("process has ended: %s", tmp);
+        goto cleanup;
+    } else if (ret == 0) {
+        VIR_DEBUG("trying SIGTERM to child process %d", cmd->pid);
+        kill(cmd->pid, SIGTERM);
+        usleep(10 * 1000);
+        while ((ret = waitpid(cmd->pid, &status, WNOHANG)) == -1 &&
+               errno == EINTR);
+        if (ret == cmd->pid) {
+            tmp = virCommandTranslateStatus(status);
+            VIR_DEBUG("process has ended: %s", tmp);
+            goto cleanup;
+        } else if (ret == 0) {
+            VIR_DEBUG("trying SIGKILL to child process %d", cmd->pid);
+            kill(cmd->pid, SIGKILL);
+            while ((ret = waitpid(cmd->pid, &status, 0)) == -1 &&
+                   errno == EINTR);
+            if (ret == cmd->pid) {
+                tmp = virCommandTranslateStatus(status);
+                VIR_DEBUG("process has ended: %s", tmp);
+                goto cleanup;
+            }
+        }
+    }
+    VIR_DEBUG("failed to reap child %d, abandoning it", cmd->pid);
+
+cleanup:
+    VIR_FREE(tmp);
+    cmd->pid = -1;
+    cmd->reap = false;
+    errno = saved_errno;
+}
+
+/*
  * Release all resources
  */
 void
@@ -1320,5 +1383,8 @@ virCommandFree(virCommandPtr cmd)

     VIR_FREE(cmd->pidfile);

+    if (cmd->reap)
+        virCommandAbort(cmd);
+
     VIR_FREE(cmd);
 }
diff --git a/src/util/command.h b/src/util/command.h
index e4027e5..ff8ccf5 100644
--- a/src/util/command.h
+++ b/src/util/command.h
@@ -275,7 +275,17 @@ int virCommandWait(virCommandPtr cmd,
                    int *exitstatus) ATTRIBUTE_RETURN_CHECK;

 /*
- * Release all resources
+ * Abort an async command if it is running, without issuing
+ * any errors or affecting errno.  Designed for error paths
+ * where some but not all paths to the cleanup code might
+ * have started the child process.
+ */
+void virCommandAbort(virCommandPtr cmd);
+
+/*
+ * Release all resources.  The only exception is that if you called
+ * virCommandRunAsync with a non-null pid, then the asynchronous child
+ * is not reaped, and you must call waitpid() yourself.
  */
 void virCommandFree(virCommandPtr cmd);

diff --git a/tests/commandtest.c b/tests/commandtest.c
index dc2f8a1..527b95a 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -708,6 +708,42 @@ cleanup:
     return ret;
 }

+/*
+ * Asynchronously run long-running daemon, to ensure no hang.
+ */
+static int test19(const void *unused ATTRIBUTE_UNUSED)
+{
+    virCommandPtr cmd = virCommandNewArgList("sleep", "100", NULL);
+    pid_t pid;
+    int ret = -1;
+
+    alarm(5);
+    if (virCommandRunAsync(cmd, &pid) < 0) {
+        virErrorPtr err = virGetLastError();
+        printf("Cannot run child %s\n", err->message);
+        goto cleanup;
+    }
+
+    if (kill(pid, 0) != 0) {
+        printf("Child should still be running");
+        goto cleanup;
+    }
+
+    virCommandAbort(cmd);
+
+    if (kill(pid, 0) == 0) {
+        printf("Child should be aborted");
+        goto cleanup;
+    }
+
+    alarm(0);
+
+    ret = 0;
+
+cleanup:
+    virCommandFree(cmd);
+    return ret;
+}

 static int
 mymain(int argc, char **argv)
@@ -781,6 +817,7 @@ mymain(int argc, char **argv)
     DO_TEST(test16);
     DO_TEST(test17);
     DO_TEST(test18);
+    DO_TEST(test19);

     return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
 }
-- 
1.7.4