2:36 p.m.
When using virCommandRunAsync and saving the pid for later, it
is useful to be able to reap that pid in the same way that it
would have been auto-reaped by virCommand if we had passed
NULL for the pid argument in the first place.
* src/util/command.c (virPidWait, virPidAbort): New functions,
created from...
(virCommandWait, virCommandAbort): ...bodies of these.
(includes): Drop duplicate <stdlib.h>. Ensure that our pid_t
assumptions hold.
(virCommandRunAsync): Improve documentation.
* src/util/command.h (virPidWait, virPidAbort): New prototypes.
* src/libvirt_private.syms: Export them.
* docs/internals/command.html.in: Document them.
---
docs/internals/command.html.in | 17 +++++
src/libvirt_private.syms | 2 +
src/util/command.c | 140 ++++++++++++++++++++++++++++------------
src/util/command.h | 28 ++++++++
4 files changed, 146 insertions(+), 41 deletions(-)
diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 27dcf9c..8a194ec 100644
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -497,6 +497,23 @@
error if not.
</p>
+ <p>
+ There are two approaches to child process cleanup, determined by
+ how long you want to keep the virCommand object in scope.
+ </p>
+
+ <p>1. If the virCommand object will outlast the child process,
+ then pass NULL for the pid argument, and the child process will
+ automatically be reaped at virCommandFree, unless you reap it
+ sooner via virCommandWait or virCommandAbort.
+ </p>
+
+ <p>2. If the child process must exist on at least one code path
+ after virCommandFree, then pass a pointer for the pid argument.
+ Later, to clean up the child, call virPidWait or virPidAbort.
+ Before virCommandFree, you can still use virCommandWait or
+ virCommandAbort to reap the process.
+ </p>
<h3><a name="release">Releasing resources</a></h3>
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3237d18..f95d341 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -134,6 +134,8 @@ virCommandTranslateStatus;
virCommandWait;
virCommandWriteArgLog;
virFork;
+virPidAbort;
+virPidWait;
virRun;
diff --git a/src/util/command.c b/src/util/command.c
index eae58b2..c194fb1 100644
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -41,8 +41,7 @@
#include "files.h"
#include "buf.h"
#include "ignore-value.h"
-
-#include <stdlib.h>
+#include "verify.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -50,6 +49,9 @@
virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \
__FUNCTION__, __LINE__, __VA_ARGS__)
+/* We have quite a bit of changes to make if this doesn't hold. */
+verify(sizeof(pid_t) <= sizeof(int));
+
/* Flags for virExecWithHook */
enum {
VIR_EXEC_NONE = 0,
@@ -1897,6 +1899,48 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
/*
+ * Wait for a child process to complete.
+ * Return -1 on any error waiting for
+ * completion. Returns 0 if the command
+ * finished with the exit status set
+ */
+int
+virPidWait(pid_t pid, int *exitstatus)
+{
+ int ret;
+ int status;
+
+ if (pid <= 0) {
+ virReportSystemError(EINVAL, _("unable to wait for process %d"), pid);
+ return -1;
+ }
+
+ /* Wait for intermediate process to exit */
+ while ((ret = waitpid(pid, &status, 0)) == -1 &&
+ errno == EINTR);
+
+ if (ret == -1) {
+ virReportSystemError(errno, _("unable to wait for process %d"), pid);
+ return -1;
+ }
+
+ if (exitstatus == NULL) {
+ if (status != 0) {
+ char *st = virCommandTranslateStatus(status);
+ virCommandError(VIR_ERR_INTERNAL_ERROR,
+ _("Child process (%d) status unexpected: %s"),
+ pid, NULLSTR(st));
+ VIR_FREE(st);
+ return -1;
+ }
+ } else {
+ *exitstatus = status;
+ }
+
+ return 0;
+}
+
+/*
* Wait for the async command to complete.
* Return -1 on any error waiting for
* completion. Returns 0 if the command
@@ -1906,7 +1950,7 @@ int
virCommandWait(virCommandPtr cmd, int *exitstatus)
{
int ret;
- int status;
+ int status = 0;
if (!cmd ||cmd->has_error == ENOMEM) {
virReportOOMError();
@@ -1924,22 +1968,17 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
return -1;
}
-
- /* Wait for intermediate process to exit */
- while ((ret = waitpid(cmd->pid, &status, 0)) == -1 &&
- errno == EINTR);
-
- if (ret == -1) {
- virReportSystemError(errno, _("unable to wait for process %d"),
- cmd->pid);
- return -1;
- }
-
- cmd->pid = -1;
- cmd->reap = false;
-
- if (exitstatus == NULL) {
- if (status != 0) {
+ /* If virPidWait reaps pid but then returns failure because
+ * exitstatus was NULL, then a second virCommandWait would risk
+ * calling waitpid on an unrelated process. Besides, that error
+ * message is not as detailed as what we can provide. So, we
+ * guarantee that virPidWait only fails due to failure to wait,
+ * and repeat the exitstatus check code ourselves. */
+ ret = virPidWait(cmd->pid, exitstatus ? exitstatus : &status);
+ if (ret == 0) {
+ cmd->pid = -1;
+ cmd->reap = false;
+ if (status) {
char *str = virCommandToString(cmd);
char *st = virCommandTranslateStatus(status);
virCommandError(VIR_ERR_INTERNAL_ERROR,
@@ -1949,75 +1988,94 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
VIR_FREE(st);
return -1;
}
- } else {
- *exitstatus = status;
}
- return 0;
+ return ret;
}
#ifndef WIN32
/*
- * 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.
+ * Abort a child process if PID is positive and that child is still
+ * 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)
+virPidAbort(pid_t pid)
{
int saved_errno;
int ret;
int status;
char *tmp = NULL;
- if (!cmd || cmd->pid == -1)
+ if (pid <= 0)
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 &&
+ VIR_DEBUG("aborting child process %d", pid);
+ while ((ret = waitpid(pid, &status, WNOHANG)) == -1 &&
errno == EINTR);
- if (ret == cmd->pid) {
+ if (ret == 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);
+ VIR_DEBUG("trying SIGTERM to child process %d", pid);
+ kill(pid, SIGTERM);
usleep(10 * 1000);
- while ((ret = waitpid(cmd->pid, &status, WNOHANG)) == -1 &&
+ while ((ret = waitpid(pid, &status, WNOHANG)) == -1 &&
errno == EINTR);
- if (ret == cmd->pid) {
+ if (ret == 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 &&
+ VIR_DEBUG("trying SIGKILL to child process %d", pid);
+ kill(pid, SIGKILL);
+ while ((ret = waitpid(pid, &status, 0)) == -1 &&
errno == EINTR);
- if (ret == cmd->pid) {
+ if (ret == pid) {
tmp = virCommandTranslateStatus(status);
VIR_DEBUG("process has ended: %s", tmp);
goto cleanup;
}
}
}
- VIR_DEBUG("failed to reap child %d, abandoning it", cmd->pid);
+ VIR_DEBUG("failed to reap child %d, abandoning it", pid);
cleanup:
VIR_FREE(tmp);
+ errno = saved_errno;
+}
+
+/*
+ * 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)
+{
+ if (!cmd || cmd->pid == -1)
+ return;
+ virPidAbort(cmd->pid);
cmd->pid = -1;
cmd->reap = false;
- errno = saved_errno;
}
#else /* WIN32 */
void
+virPidAbort(pid_t pid)
+{
+ /* Not yet ported to mingw. Any volunteers? */
+ VIR_DEBUG("failed to reap child %d, abandoning it", pid);
+}
+
+void
virCommandAbort(virCommandPtr cmd ATTRIBUTE_UNUSED)
{
/* Mingw lacks WNOHANG and kill(). But since we haven't ported
diff --git a/src/util/command.h b/src/util/command.h
index e9f4227..c8a04f1 100644
--- a/src/util/command.h
+++ b/src/util/command.h
@@ -292,11 +292,31 @@ int virCommandRun(virCommandPtr cmd,
* Run the command asynchronously
* Returns -1 on any error executing the
* command. Returns 0 if the command executed.
+ *
+ * There are two approaches to child process cleanup.
+ * 1. Use auto-cleanup, by passing NULL for pid. The child will be
+ * auto-reaped by virCommandFree, unless you reap it earlier via
+ * virCommandWait or virCommandAbort. Good for where cmd is in
+ * scope for the duration of the child process.
+ * 2. Use manual cleanup, by passing the address of a pid_t variable
+ * for pid. While cmd is still in scope, you may reap the child via
+ * virCommandWait or virCommandAbort. But after virCommandFree, if
+ * you have not yet reaped the child, then it continues to run until
+ * you call virPidWait or virPidAbort.
*/
int virCommandRunAsync(virCommandPtr cmd,
pid_t *pid) ATTRIBUTE_RETURN_CHECK;
/*
+ * Wait for a child process to complete.
+ * Return -1 on any error waiting for
+ * completion. Returns 0 if the command
+ * finished with the exit status set.
+ */
+int virPidWait(pid_t pid,
+ int *exitstatus) ATTRIBUTE_RETURN_CHECK;
+
+/*
* Wait for the async command to complete.
* Return -1 on any error waiting for
* completion. Returns 0 if the command
@@ -328,6 +348,14 @@ int virCommandHandshakeNotify(virCommandPtr cmd)
ATTRIBUTE_RETURN_CHECK;
/*
+ * Abort a child process if PID is positive and that child is still
+ * 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 virPidAbort(pid_t pid);
+
+/*
* 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
--
1.7.4.4