Improve virExec error reporting

diff --git a/ChangeLog b/ChangeLog
index a7aad45cf72c9bcc5a4733e3837123965e95ac16..b2f6ebb00aff8d9b4c6e3f296b97d21d30635fda 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+Wed Aug 20 09:28:33 BST 2008 Daniel P. Berrange <berrange@redhat.com>
+
+       * src/util.c: Re-arrange virExec() to improve error reporting
+
 Mon Aug 18 10:22:33 BST 2008 Daniel P. Berrange <berrange@redhat.com>
 
        * src/libvirt.c: Remove duplicate call to virInitialize() in

diff --git a/src/util.c b/src/util.c
index a4ae78a4e0b8aea426c4c2c932c1353bf4a237c6..a0b02c120ec746adc593b2d20f7b74e04f5ab080 100644 (file)
--- a/src/util.c
+++ b/src/util.c
@@ -64,9 +64,12 @@
 #ifndef PROXY
 static void
 ReportError(virConnectPtr conn,
-                      virDomainPtr dom,
-                      virNetworkPtr net,
-                      int code, const char *fmt, ...) {
+            int code, const char *fmt, ...)
+    ATTRIBUTE_FORMAT(printf, 3, 4);
+
+static void
+ReportError(virConnectPtr conn,
+            int code, const char *fmt, ...) {
     va_list args;
     char errorMessage[MAX_ERROR_LEN];
 
@@ -77,7 +80,7 @@ ReportError(virConnectPtr conn,
     } else {
         errorMessage[0] = '\0';
     }
-    __virRaiseError(conn, dom, net, VIR_FROM_NONE, code, VIR_ERR_ERROR,
+    __virRaiseError(conn, NULL, NULL, VIR_FROM_NONE, code, VIR_ERR_ERROR,
                     NULL, NULL, NULL, -1, -1, "%s", errorMessage);
 }
 
@@ -112,7 +115,7 @@ _virExec(virConnectPtr conn,
     int pipeerr[2] = {-1,-1};
 
     if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) {
-        ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
                     _("cannot open %s: %s"),
                     _PATH_DEVNULL, strerror(errno));
         goto cleanup;
@@ -120,13 +123,41 @@ _virExec(virConnectPtr conn,
 
     if ((outfd != NULL && pipe(pipeout) < 0) ||
         (errfd != NULL && pipe(pipeerr) < 0)) {
-        ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
                     _("cannot create pipe: %s"), strerror(errno));
         goto cleanup;
     }
 
+    if (outfd) {
+        if(non_block &&
+           virSetNonBlock(pipeout[0]) == -1) {
+            ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                        _("Failed to set non-blocking file descriptor flag"));
+            goto cleanup;
+        }
+
+        if(virSetCloseExec(pipeout[0]) == -1) {
+            ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                        _("Failed to set close-on-exec file descriptor flag"));
+            goto cleanup;
+        }
+    }
+    if (errfd) {
+        if(non_block &&
+           virSetNonBlock(pipeerr[0]) == -1) {
+            ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                        _("Failed to set non-blocking file descriptor flag"));
+            goto cleanup;
+        }
+        if(virSetCloseExec(pipeerr[0]) == -1) {
+            ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                        _("Failed to set close-on-exec file descriptor flag"));
+            goto cleanup;
+        }
+    }
+
     if ((pid = fork()) < 0) {
-        ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
                     _("cannot fork child process: %s"), strerror(errno));
         goto cleanup;
     }
@@ -135,26 +166,10 @@ _virExec(virConnectPtr conn,
         close(null);
         if (outfd) {
             close(pipeout[1]);
-            if(non_block)
-                if(virSetNonBlock(pipeout[0]) == -1)
-                    ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                        _("Failed to set non-blocking file descriptor flag"));
-
-            if(virSetCloseExec(pipeout[0]) == -1)
-                ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                        _("Failed to set close-on-exec file descriptor flag"));
             *outfd = pipeout[0];
         }
         if (errfd) {
             close(pipeerr[1]);
-            if(non_block)
-                if(virSetNonBlock(pipeerr[0]) == -1)
-                    ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                          _("Failed to set non-blocking file descriptor flag"));
-
-            if(virSetCloseExec(pipeerr[0]) == -1)
-                ReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
-                        _("Failed to set close-on-exec file descriptor flag"));
             *errfd = pipeerr[0];
         }
         *retpid = pid;
@@ -163,23 +178,47 @@ _virExec(virConnectPtr conn,
 
     /* child */
 
-    if (pipeout[0] > 0 && close(pipeout[0]) < 0)
-        _exit(1);
-    if (pipeerr[0] > 0 && close(pipeerr[0]) < 0)
-        _exit(1);
+    /* Don't want to report errors against this accidentally, so
+       just discard it */
+    conn = NULL;
+    /* Remove any error callback too, so errors in child now
+       get sent to stderr where they stand a fighting chance
+       of being seen / logged */
+    virSetErrorFunc(NULL, NULL);
 
-    if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0)
+    if (pipeout[0] > 0)
+        close(pipeout[0]);
+    if (pipeerr[0] > 0)
+        close(pipeerr[0]);
+
+
+    if (dup2(infd >= 0 ? infd : null, STDIN_FILENO) < 0) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("failed to setup stdin file handle: %s"), strerror(errno));
         _exit(1);
+    }
 #ifndef ENABLE_DEBUG
-    if (dup2(pipeout[1] > 0 ? pipeout[1] : null, STDOUT_FILENO) < 0)
+    if (dup2(pipeout[1] > 0 ? pipeout[1] : null, STDOUT_FILENO) < 0) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("failed to setup stdout file handle: %s"), strerror(errno));
         _exit(1);
-    if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0)
+    }
+    if (dup2(pipeerr[1] > 0 ? pipeerr[1] : null, STDERR_FILENO) < 0) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("failed to setup stderr file handle: %s"), strerror(errno));
         _exit(1);
+    }
 #else /* ENABLE_DEBUG */
-    if (pipeout[1] > 0 && dup2(pipeout[1], STDOUT_FILENO) < 0)
+    if (pipeout[1] > 0 && dup2(pipeout[1], STDOUT_FILENO) < 0) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("failed to setup stderr file handle: %s"), strerror(errno));
         _exit(1);
-    if (pipeerr[1] > 0 && dup2(pipeerr[1], STDERR_FILENO) < 0)
+    }
+    if (pipeerr[1] > 0 && dup2(pipeerr[1], STDERR_FILENO) < 0) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("failed to setup stdout file handle: %s"), strerror(errno));
         _exit(1);
+    }
 #endif /* ENABLE_DEBUG */
 
     close(null);
@@ -190,11 +229,21 @@ _virExec(virConnectPtr conn,
 
     execvp(argv[0], (char **) argv);
 
+    ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                _("cannot execute binary '%s': %s"),
+                argv[0], strerror(errno));
+
     _exit(1);
 
     return 0;
 
  cleanup:
+    /* This is cleanup of parent process only - child
+       should never jump here on error */
+
+    /* NB we don't ReportError() on any failures here
+       because the code which jumped hre already raised
+       an error condition which we must not overwrite */
     if (pipeerr[0] > 0)
         close(pipeerr[0]);
     if (pipeerr[1] > 0)
@@ -249,12 +298,24 @@ virRun(virConnectPtr conn,
         return ret;
 
     while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR);
-    if (ret == -1)
+    if (ret == -1) {
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("cannot wait for '%s': %s"),
+                    argv[0], strerror(errno));
         return -1;
+    }
 
     if (status == NULL) {
         errno = EINVAL;
-        return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1;
+        if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0)
+            return 0;
+
+        ReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                    _("%s exited with non-zero status %d and signal %d"),
+                    argv[0],
+                    WIFEXITED(exitstatus) ? WEXITSTATUS(exitstatus) : 0,
+                    WIFSIGNALED(exitstatus) ? WTERMSIG(exitstatus) : 0);
+        return -1;
     } else {
         *status = exitstatus;
         return 0;
@@ -271,7 +332,7 @@ virExec(virConnectPtr conn,
         int *outfd ATTRIBUTE_UNUSED,
         int *errfd ATTRIBUTE_UNUSED)
 {
-    ReportError (conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+    ReportError (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
     return -1;
 }
 
@@ -283,7 +344,7 @@ virExecNonBlock(virConnectPtr conn,
                 int *outfd ATTRIBUTE_UNUSED,
                 int *errfd ATTRIBUTE_UNUSED)
 {
-    ReportError (conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+    ReportError (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
     return -1;
 }