[libvirt] [PATCH] command: properly diagnose process exit via signal
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. --- In response to my finding here: https://www.redhat.com/archives/libvir-list/2011-March/msg01029.html
status includes normal exit and signals. This should probably use WIFEXITED and WEXITSTATUS to avoid printing values shifted by 8. For that matter, I just noticed that virCommandWait should probably be more careful in how it interprets status.
daemon/remote.c | 11 +++++++- 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 | 7 +++++- src/util/util.c | 27 ++++++++++------------ 8 files changed, 92 insertions(+), 44 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index f410982..2c54721 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__) @@ -4363,8 +4364,14 @@ 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); + if (!tmp) { + virReportOOMError(); + goto authfail; + } + VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d: %s"), + action, callerPid, callerUid, 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 55be36e..d2f98fc 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 22f350b..89f82c7 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 */ @@ -1226,10 +1252,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..0e890d0 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 @@ -273,5 +273,10 @@ int virCommandWait(virCommandPtr cmd, */ void virCommandFree(virCommandPtr cmd); +/* + * Translate an exit status into a malloc'd string. + */ +char *virCommandTranslateStatus(int exitstatus) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_COMMAND_H__ */ 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
* src/util/command.c (virCommandRunAsync): Since virExec only creates pidfiles for daemon, enforce this in virCommand. --- src/util/command.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 89f82c7..7b4337f 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1172,7 +1172,11 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) cmd->pwd); return -1; } - + if (cmd->pidfile && !(cmd->flags & VIR_EXEC_DAEMON)) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("creation of pid file requires daemonized command")); + return -1; + } str = virCommandToString(cmd); VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); -- 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. * 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 Tue, Mar 22, 2011 at 04:45:09PM -0600, Eric Blake wrote:
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.
/* @@ -1207,6 +1208,8 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
if (ret == 0 && pid) *pid = cmd->pid; + else + cmd->reap = true;
return ret; }
[snip]
@@ -1305,5 +1368,8 @@ virCommandFree(virCommandPtr cmd)
VIR_FREE(cmd->pidfile);
+ if (cmd->reap) + virCommandAbort(cmd); + VIR_FREE(cmd); }
We allow virCommandRunAsync to be used for daemonized commands, so I don't think it is safe to unconditionally kill off the PID when free'ing the virCommandPtr instance. Regards, 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 :|
On 03/23/2011 06:19 AM, Daniel P. Berrange wrote:
On Tue, Mar 22, 2011 at 04:45:09PM -0600, Eric Blake wrote:
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.
/* @@ -1207,6 +1208,8 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
if (ret == 0 && pid) *pid = cmd->pid; + else + cmd->reap = true;
return ret; }
[snip]
@@ -1305,5 +1368,8 @@ virCommandFree(virCommandPtr cmd)
VIR_FREE(cmd->pidfile);
+ if (cmd->reap) + virCommandAbort(cmd); + VIR_FREE(cmd); }
We allow virCommandRunAsync to be used for daemonized commands, so I don't think it is safe to unconditionally kill off the PID when free'ing the virCommandPtr instance.
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/23/2011 06:30 AM, Eric Blake wrote:
We allow virCommandRunAsync to be used for daemonized commands, so I don't think it is safe to unconditionally kill off the PID when free'ing the virCommandPtr instance.
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).
In fact, I can mandate virCommandRun with daemonized children, to make this impossible to reap the wrong child; v2 coming up later with that change. -- Eric Blake eblake@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:
* src/util/command.c (virCommandRunAsync): Since virExec only creates pidfiles for daemon, enforce this in virCommand. --- src/util/command.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index 89f82c7..7b4337f 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1172,7 +1172,11 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) cmd->pwd); return -1; } - + if (cmd->pidfile && !(cmd->flags & VIR_EXEC_DAEMON)) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("creation of pid file requires daemonized command")); + return -1; + }
str = virCommandToString(cmd); VIR_DEBUG("About to run %s", str ? str : cmd->args[0]);
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 :|
On 03/23/2011 06:24 AM, Daniel P. Berrange wrote:
On Tue, Mar 22, 2011 at 04:45:08PM -0600, Eric Blake wrote:
* src/util/command.c (virCommandRunAsync): Since virExec only creates pidfiles for daemon, enforce this in virCommand. --- src/util/command.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/command.c b/src/util/command.c index 89f82c7..7b4337f 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1172,7 +1172,11 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) cmd->pwd); return -1; } - + if (cmd->pidfile && !(cmd->flags & VIR_EXEC_DAEMON)) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("creation of pid file requires daemonized command")); + return -1; + }
str = virCommandToString(cmd); VIR_DEBUG("About to run %s", str ? str : cmd->args[0]);
ACK
Thanks; I've pushed this one. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 22, 2011 at 12:00:02PM -0600, Eric Blake wrote:
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. ---
In response to my finding here: https://www.redhat.com/archives/libvir-list/2011-March/msg01029.html
status includes normal exit and signals. This should probably use WIFEXITED and WEXITSTATUS to avoid printing values shifted by 8. For that matter, I just noticed that virCommandWait should probably be more careful in how it interprets status.
daemon/remote.c | 11 +++++++- 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 | 7 +++++- src/util/util.c | 27 ++++++++++------------ 8 files changed, 92 insertions(+), 44 deletions(-)
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); + if (!tmp) { + virReportOOMError(); + goto authfail; + } + VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d: %s"), + action, callerPid, callerUid, tmp); + VIR_FREE(tmp); goto authdeny;
Hmm, I rather think we ought to keep the VIR_ERROR log message, even if virCommandTranslateStatus does OOM. eg just use NULLSTR(tmp) 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 :|
Should address findings from v1. Eric Blake (3): command: properly diagnose process exit via signal command: don't mix RunAsync and daemons command: add virCommandAbort for cleanup paths daemon/remote.c | 7 +- src/libvirt_private.syms | 2 + src/nwfilter/nwfilter_ebiptables_driver.c | 16 +++- src/security/security_apparmor.c | 22 ++++-- src/storage/storage_backend.c | 18 ++--- src/util/command.c | 115 ++++++++++++++++++++++++++++- src/util/command.h | 20 +++++- src/util/util.c | 27 +++---- tests/commandtest.c | 37 +++++++++ 9 files changed, 219 insertions(+), 45 deletions(-) -- 1.7.4
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
On Wed, Mar 23, 2011 at 05:48:11PM -0600, Eric Blake wrote:
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.
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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
On Wed, Mar 23, 2011 at 05:48:12PM -0600, Eric Blake wrote:
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
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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
On 03/23/2011 05:48 PM, Eric Blake wrote:
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.
Actually, I'm going to squash this in, and make v2 different than v1 by enhancing the tests for daemons: diff --git i/tests/commandtest.c w/tests/commandtest.c index 527b95a..c313a2c 100644 --- i/tests/commandtest.c +++ w/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); Hmm, should we have a virCommandRunDaemon(cmd, pid_t*) helper function that runs a command as a daemon and returns its pid by reading a temporary pidfile? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Mar 23, 2011 at 05:48:13PM -0600, Eric Blake wrote:
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(-)
ACK, including the followup patch to tests/commandtest.c, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/24/2011 09:16 PM, Daniel Veillard wrote:
On Wed, Mar 23, 2011 at 05:48:13PM -0600, Eric Blake wrote:
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(-)
ACK, including the followup patch to tests/commandtest.c,
Series pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake