6:48 p.m.
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