[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 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