[libvirt] [PATCH] command: properly diagnose process exit via signal
Eric Blake
eblake at redhat.com
Tue Mar 22 18:00:02 UTC 2011
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
More information about the libvir-list
mailing list