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

@@ -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",

src/libvirt_private.syms

@@ -123,6 +123,7 @@ virCommandSetPreExecHook;
 virCommandSetWorkingDirectory;
 virCommandToString;
 virCommandTransferFD;
+virCommandTranslateStatus;
 virCommandWait;
 virCommandWriteArgLog;
 

src/nwfilter/nwfilter_ebiptables_driver.c

 /*
  * 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);
 

src/security/security_apparmor.c

 /*
  * 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;
     }
 

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);
         }
     }
 

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 {

src/util/command.h

 /*
  * 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

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;
     }