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. * 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. --- 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 fe70bf3..c2df20b 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 7b4337f..bb7f996 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -76,6 +76,7 @@ struct _virCommand { pid_t pid; char *pidfile; + bool reap; }; /* @@ -1207,6 +1208,8 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) if (ret == 0 && pid) *pid = cmd->pid; + else + cmd->reap = true; return ret; } @@ -1252,6 +1255,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) } cmd->pid = -1; + cmd->reap = false; if (exitstatus == NULL) { if (status != 0) { @@ -1273,6 +1277,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 @@ -1305,5 +1368,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

On 03/23/2011 06:19 AM, Daniel P. Berrange wrote: No, we don't. 'git grep virCommandRunAsync' shows that the only current client is virsh.c. virCommandDaemonize is run synchronously (the daemon grandchild is run async, but the intermediate child runs to completion via virCommandRun). -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Tue, Mar 22, 2011 at 04:45:08PM -0600, Eric Blake wrote: ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Child processes don't always reach _exit(); if they die from a signal, then any messages should still be accurate. Most users either expect a 0 status (thankfully, if status==0, then WIFEXITED(status) is true and WEXITSTATUS(status)==0 for all known platforms) or were filtering on WIFEXITED before printing a status, but a few were missing this check. Additionally, nwfilter_ebiptables_driver was making an assumption that works on Linux (where WEXITSTATUS shifts and WTERMSIG just masks) but fails on other platforms (where WEXITSTATUS just masks and WTERMSIG shifts). * src/util/command.h (virCommandTranslateStatus): New helper. * src/libvirt_private.syms (command.h): Export it. * src/util/command.c (virCommandTranslateStatus): New function. (virCommandWait): Use it to also diagnose status from signals. * src/security/security_apparmor.c (load_profile): Likewise. * src/storage/storage_backend.c (virStorageBackendQEMUImgBackingFormat): Likewise. * src/util/util.c (virExecDaemonize, virRunWithHook) (virFileOperation, virDirCreate): Likewise. * daemon/remote.c (remoteDispatchAuthPolkit): Likewise. * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesExecCLI): Likewise. --- v2: in daemon/remote.c, don't fail to log a minimal error on OOM. daemon/remote.c | 7 ++++- src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_ebiptables_driver.c | 16 ++++++++++--- src/security/security_apparmor.c | 22 ++++++++++++------ src/storage/storage_backend.c | 18 ++++++--------- src/util/command.c | 34 ++++++++++++++++++++++++++-- src/util/command.h | 8 ++++++- src/util/util.c | 27 ++++++++++------------ 8 files changed, 89 insertions(+), 44 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 7520df3..d49e0d8 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -60,6 +60,7 @@ #include "uuid.h" #include "network.h" #include "libvirt/libvirt-qemu.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_REMOTE #define REMOTE_DEBUG(fmt, ...) VIR_DEBUG(fmt, __VA_ARGS__) @@ -4368,8 +4369,10 @@ remoteDispatchAuthPolkit (struct qemud_server *server, goto authfail; } if (status != 0) { - VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d, result: %d"), - action, callerPid, callerUid, status); + char *tmp = virCommandTranslateStatus(status); + VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d: %s"), + action, callerPid, callerUid, NULLSTR(tmp)); + VIR_FREE(tmp); goto authdeny; } PROBE(CLIENT_AUTH_ALLOW, "fd=%d, auth=%d, username=%s", diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 63b6af7..f783c63 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -123,6 +123,7 @@ virCommandSetPreExecHook; virCommandSetWorkingDirectory; virCommandToString; virCommandTransferFD; +virCommandTranslateStatus; virCommandWait; virCommandWriteArgLog; diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 2ec5b02..75fddfb 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -1,6 +1,7 @@ /* * nwfilter_ebiptables_driver.c: driver for ebtables/iptables on tap devices * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corp. * Copyright (C) 2010 Stefan Berger * @@ -2558,7 +2559,7 @@ err_exit: * ebiptablesExecCLI: * @buf : pointer to virBuffer containing the string with the commands to * execute. - * @status: Pointer to an integer for returning the status of the + * @status: Pointer to an integer for returning the WEXITSTATUS of the * commands executed via the script the was run. * * Returns 0 in case of success, != 0 in case of an error. The returned @@ -2587,7 +2588,7 @@ ebiptablesExecCLI(virBufferPtr buf, cmds = virBufferContentAndReset(buf); - VIR_DEBUG("%s", cmds); + VIR_DEBUG("%s", NULLSTR(cmds)); if (!cmds) return 0; @@ -2606,9 +2607,16 @@ ebiptablesExecCLI(virBufferPtr buf, virMutexUnlock(&execCLIMutex); - *status >>= 8; + if (rc == 0) { + if (WIFEXITED(*status)) { + *status = WEXITSTATUS(*status); + } else { + rc = -1; + *status = 1; + } + } - VIR_DEBUG("rc = %d, status = %d",rc, *status); + VIR_DEBUG("rc = %d, status = %d", rc, *status); unlink(filename); diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index deb4181..3edc680 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1,5 +1,6 @@ /* * AppArmor security driver for libvirt + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2009-2010 Canonical Ltd. * * This library is free software; you can redistribute it and/or @@ -36,6 +37,7 @@ #include "hostusb.h" #include "files.h" #include "configmake.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_APPARMOR_VOID_DOI "0" @@ -217,15 +219,19 @@ load_profile(virSecurityManagerPtr mgr, VIR_FORCE_CLOSE(pipefd[1]); rc = 0; - rewait: - if (waitpid(child, &status, 0) != child) { - if (errno == EINTR) - goto rewait; - + while ((ret = waitpid(child, &status, 0)) < 0 && errno == EINTR); + if (ret < 0) { + virReportSystemError(errno, + _("Failed to reap virt-aa-helper pid %lu"), + (unsigned long)child); + rc = -1; + } else if (status) { + char *str = virCommandTranslateStatus(status); virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected exit status from virt-aa-helper " - "%d pid %lu"), - WEXITSTATUS(status), (unsigned long)child); + _("Unexpected status from virt-aa-helper " + "pid %lu: %s"), + (unsigned long)child, NULLSTR(str)); + VIR_FREE(str); rc = -1; } diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index fc08c68..c6c16c8 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -56,6 +56,7 @@ #include "storage_backend.h" #include "logging.h" #include "files.h" +#include "command.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -631,18 +632,13 @@ static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) cleanup: VIR_FREE(help); VIR_FORCE_CLOSE(newstdout); -rewait: if (child) { - if (waitpid(child, &status, 0) != child) { - if (errno == EINTR) - goto rewait; - - VIR_ERROR(_("Unexpected exit status from qemu %d pid %lu"), - WEXITSTATUS(status), (unsigned long)child); - } - if (WEXITSTATUS(status) != 0) { - VIR_WARN("Unexpected exit status '%d', qemu probably failed", - WEXITSTATUS(status)); + while (waitpid(child, &status, 0) == -1 && errno == EINTR); + if (status) { + tmp = virCommandTranslateStatus(status); + VIR_WARN("Unexpected status, qemu probably failed: %s", + NULLSTR(tmp)); + VIR_FREE(tmp); } } diff --git a/src/util/command.c b/src/util/command.c index a33d333..7b4337f 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -798,6 +798,26 @@ virCommandToString(virCommandPtr cmd) /* + * Translate an exit status into a malloc'd string. Generic helper + * for virCommandRun and virCommandWait status argument, as well as + * raw waitpid and older virRun status. + */ +char * +virCommandTranslateStatus(int status) +{ + char *buf; + if (WIFEXITED(status)) { + virAsprintf(&buf, _("exit status %d"), WEXITSTATUS(status)); + } else if (WIFSIGNALED(status)) { + virAsprintf(&buf, _("fatal signal %d"), WTERMSIG(status)); + } else { + virAsprintf(&buf, _("invalid value %d"), status); + } + return buf; +} + + +/* * Manage input and output to the child process. */ static int @@ -958,6 +978,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) struct stat st; bool string_io; bool async_io = false; + char *str; if (!cmd ||cmd->has_error == ENOMEM) { virReportOOMError(); @@ -1048,9 +1069,14 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) if (virCommandWait(cmd, exitstatus) < 0) ret = -1; - VIR_DEBUG("Result stdout: '%s' stderr: '%s'", + str = (exitstatus ? virCommandTranslateStatus(*exitstatus) + : (char *) "status 0"); + VIR_DEBUG("Result %s, stdout: '%s' stderr: '%s'", + NULLSTR(str), cmd->outbuf ? NULLSTR(*cmd->outbuf) : "(null)", cmd->errbuf ? NULLSTR(*cmd->errbuf) : "(null)"); + if (exitstatus) + VIR_FREE(str); /* Reset any capturing, in case caller runs * this identical command again */ @@ -1230,10 +1256,12 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) if (exitstatus == NULL) { if (status != 0) { char *str = virCommandToString(cmd); + char *st = virCommandTranslateStatus(status); virCommandError(VIR_ERR_INTERNAL_ERROR, - _("Child process (%s) exited with status %d."), - str ? str : cmd->args[0], WEXITSTATUS(status)); + _("Child process (%s) status unexpected: %s"), + str ? str : cmd->args[0], NULLSTR(st)); VIR_FREE(str); + VIR_FREE(st); return -1; } } else { diff --git a/src/util/command.h b/src/util/command.h index 59d0ee3..e4027e5 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -1,7 +1,7 @@ /* * command.h: Child command execution * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -242,6 +242,12 @@ void virCommandWriteArgLog(virCommandPtr cmd, */ char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; + +/* + * Translate an exit status into a malloc'd string. + */ +char *virCommandTranslateStatus(int exitstatus) ATTRIBUTE_RETURN_CHECK; + /* * Run the command and wait for completion. * Returns -1 on any error executing the diff --git a/src/util/util.c b/src/util/util.c index 1e4e2ab..4301b00 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -76,6 +76,7 @@ #include "threads.h" #include "verify.h" #include "files.h" +#include "command.h" #ifndef NSIG # define NSIG 32 @@ -832,9 +833,11 @@ int virExecDaemonize(const char *const*argv, errno == EINTR); if (childstat != 0) { + char *str = virCommandTranslateStatus(childstat); virUtilError(VIR_ERR_INTERNAL_ERROR, - _("Intermediate daemon process exited with status %d."), - WEXITSTATUS(childstat)); + _("Intermediate daemon process status unexpected: %s"), + NULLSTR(str)); + VIR_FREE(str); ret = -2; } @@ -903,13 +906,12 @@ virRunWithHook(const char *const*argv, if (status == NULL) { errno = EINVAL; - if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) != 0) { + if (exitstatus) { + char *str = virCommandTranslateStatus(exitstatus); virUtilError(VIR_ERR_INTERNAL_ERROR, - _("'%s' exited with non-zero status %d and " - "signal %d: %s"), argv_str, - WIFEXITED(exitstatus) ? WEXITSTATUS(exitstatus) : 0, - WIFSIGNALED(exitstatus) ? WTERMSIG(exitstatus) : 0, - (errbuf ? errbuf : "")); + _("'%s' exited unexpectedly: %s"), + argv_str, NULLSTR(str)); + VIR_FREE(str); goto error; } } else { @@ -1510,8 +1512,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode, path); goto parenterror; } - ret = -WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == -EACCES)) { + if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES) { /* fall back to the simpler method, which works better in * some cases */ return virFileOperationNoFork(path, openflags, mode, uid, gid, @@ -1630,15 +1631,11 @@ int virDirCreate(const char *path, mode_t mode, path); goto parenterror; } - ret = -WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == -EACCES)) { + if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES) { /* fall back to the simpler method, which works better in * some cases */ return virDirCreateNoFork(path, mode, uid, gid, flags); } - if (ret < 0) { - goto parenterror; - } parenterror: return ret; } -- 1.7.4

It doesn't make sense to run a daemon without synchronously waiting for the child process to reply whether the daemon has been kicked off and pidfile written yet. * src/util/command.c (VIR_EXEC_RUN_SYNC): New constant. (virCommandRun): Set temporary flag. (virCommandRunAsync): Use it to prevent async runs of intermediate child when spawning asynchronous daemon grandchild. --- v2: new patch src/util/command.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 7b4337f..3a8ffae 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -41,6 +41,11 @@ virReportErrorHelper(NULL, VIR_FROM_NONE, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) +enum { + /* Internal-use extension beyond public VIR_EXEC_ flags */ + VIR_EXEC_RUN_SYNC = 0x40000000, +}; + struct _virCommand { int has_error; /* ENOMEM on allocation failure, -1 for anything else. */ @@ -1050,6 +1055,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) } } + cmd->flags |= VIR_EXEC_RUN_SYNC; if (virCommandRunAsync(cmd, NULL) < 0) { if (cmd->inbuf) { int tmpfd = infd[0]; @@ -1139,6 +1145,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) int ret; char *str; int i; + bool synchronous = false; if (!cmd || cmd->has_error == ENOMEM) { virReportOOMError(); @@ -1150,6 +1157,9 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) return -1; } + synchronous = cmd->flags & VIR_EXEC_RUN_SYNC; + cmd->flags &= ~VIR_EXEC_RUN_SYNC; + /* Buffer management can only be requested via virCommandRun. */ if ((cmd->inbuf && cmd->infd == -1) || (cmd->outbuf && cmd->outfdptr != &cmd->outfd) || @@ -1166,6 +1176,11 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) return -1; } + if (!synchronous && (cmd->flags & VIR_EXEC_DAEMON)) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("daemonized command cannot use virCommandRunAsync")); + return -1; + } if (cmd->pwd && (cmd->flags & VIR_EXEC_DAEMON)) { virCommandError(VIR_ERR_INTERNAL_ERROR, _("daemonized command cannot set working directory %s"), -- 1.7.4

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