command: add virCommandAbort for cleanup paths

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.

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 62539cf6906ad9006373afcfb1a59ffa4ff8ce7c..11fd5c7d0215d010f8703cbb95a94dac1881db52 100644 (file)
--- 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 3a8ffaea77beb42e9d1b52f4bebb9b25f75fc51a..905256e308fbda6b17a8485dffb009ce7d173a29 100644 (file)
--- 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) {
@@ -1287,6 +1291,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
  */
@@ -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 e4027e58771b16f856839798a4d19b20a841e1d1..ff8ccf5f91f43f74c3b7b1ed7ec57e8b57a8829d 100644 (file)
--- 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 dc2f8a124b53a16786fdeb5a6eddacedd418548e..c313a2cec348e0f08ffe9c635b7dfb4c04180828 100644 (file)
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -696,6 +696,14 @@ static int test18(const void *unused ATTRIBUTE_UNUSED)
         printf("cannot read pidfile\n");
         goto cleanup;
     }
+
+    virCommandFree(cmd);
+    cmd = NULL;
+    if (kill(pid, 0) != 0) {
+        printf("daemon should still be running\n");
+        goto cleanup;
+    }
+
     while (kill(pid, SIGINT) != -1)
         usleep(100*1000);
 
@@ -708,6 +716,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 +825,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);
 }