From 208a044a547c2339e426e3ae58a8cb27807f68f2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 22 Mar 2011 11:55:45 -0600 Subject: [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. --- 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 7520df34f4..d49e0d8265 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 b4b6c63f6a..62539cf690 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 2ec5b022a0..75fddfbffc 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 deb41811fb..3edc680546 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 fc08c682ba..c6c16c85df 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 a33d3339cf..7b4337f410 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -797,6 +797,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. */ @@ -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 59d0ee3ebd..e4027e5877 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 1e4e2abb4b..4301b0092d 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; }