command: improve behavior on no output

Guarantee that outbuf/errbuf are allocated on success, even if to the
empty string.  Caller always has to free the result, and empty output
check requires checking if *outbuf=='\0'.  Makes the API easier to use
safely.  Failure is best effort allocation (some paths, like
out-of-memory, cannot allocate a buffer, but most do), so caller must
free buffer on failure.

* docs/internals/command.html.in: Update documentation.
* src/util/command.c (virCommandSetOutputBuffer)
(virCommandSetErrorBuffer, virCommandProcessIO) Guarantee empty
string on no output.
* tests/commandtest.c (test17): New test.

diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 259e68e716868092e779d44684028fd887bcc0f8..95d2b815df200aee66592549bafad7b67b75deb4 100644 (file)
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -365,9 +365,15 @@
 </pre>
 
     <p>
-      Once the command has finished executing, these buffers
-      will contain the output. It is the callers responsibility
-      to free these buffers.
+      Once the command has finished executing, these buffers will
+      contain the output.  Allocation is guaranteed if virCommandRun
+      or virCommandWait succeed (if there was no output, then the
+      buffer will contain an allocated empty string); if the command
+      failed, then the buffers usually contain a best-effort
+      allocation of collected information (however, on an
+      out-of-memory condition, the buffer may still be NULL).  The
+      caller is responsible for freeing registered buffers, since the
+      buffers are designed to persist beyond virCommandFree.
     </p>
 
     <h3><a name="directory">Setting working directory</a></h3>

diff --git a/src/util/command.c b/src/util/command.c
index 3dfd33d6fe7a77073cd6571238b76c2d779a7dd8..e39e9495ef80a295c8e5ec3bf74b01c9e6137621 100644 (file)
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -533,11 +533,15 @@ virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf)
 
 
 /*
- * Capture the child's stdout to a string buffer
+ * Capture the child's stdout to a string buffer.  *outbuf is
+ * guaranteed to be allocated after successful virCommandRun or
+ * virCommandWait, and is best-effort allocated after failed
+ * virCommandRun; caller is responsible for freeing *outbuf.
  */
 void
 virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
 {
+    *outbuf = NULL;
     if (!cmd || cmd->has_error)
         return;
 
@@ -553,11 +557,15 @@ virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
 
 
 /*
- * Capture the child's stderr to a string buffer
+ * Capture the child's stderr to a string buffer.  *errbuf is
+ * guaranteed to be allocated after successful virCommandRun or
+ * virCommandWait, and is best-effort allocated after failed
+ * virCommandRun; caller is responsible for freeing *errbuf.
  */
 void
 virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf)
 {
+    *errbuf = NULL;
     if (!cmd || cmd->has_error)
         return;
 
@@ -747,6 +755,7 @@ virCommandProcessIO(virCommandPtr cmd)
     int infd = -1, outfd = -1, errfd = -1;
     size_t inlen = 0, outlen = 0, errlen = 0;
     size_t inoff = 0;
+    int ret = 0;
 
     /* With an input buffer, feed data to child
      * via pipe */
@@ -755,12 +764,27 @@ virCommandProcessIO(virCommandPtr cmd)
         infd = cmd->inpipe;
     }
 
-    /* With out/err buffer, the outfd/errfd
-     * have been filled with an FD for us */
-    if (cmd->outbuf)
+    /* With out/err buffer, the outfd/errfd have been filled with an
+     * FD for us.  Guarantee an allocated string with partial results
+     * even if we encounter a later failure, as well as freeing any
+     * results accumulated over a prior run of the same command.  */
+    if (cmd->outbuf) {
         outfd = cmd->outfd;
-    if (cmd->errbuf)
+        if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0) {
+            virReportOOMError();
+            ret = -1;
+        }
+    }
+    if (cmd->errbuf) {
         errfd = cmd->errfd;
+        if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0) {
+            virReportOOMError();
+            ret = -1;
+        }
+    }
+    if (ret == -1)
+        goto cleanup;
+    ret = -1;
 
     for (;;) {
         int i;
@@ -791,7 +815,7 @@ virCommandProcessIO(virCommandPtr cmd)
                 continue;
             virReportSystemError(errno, "%s",
                                  _("unable to poll on child"));
-            return -1;
+            goto cleanup;
         }
 
         for (i = 0; i < nfds ; i++) {
@@ -815,7 +839,7 @@ virCommandProcessIO(virCommandPtr cmd)
                         errno != EAGAIN) {
                         virReportSystemError(errno, "%s",
                                              _("unable to write to child input"));
-                        return -1;
+                        goto cleanup;
                     }
                 } else if (done == 0) {
                     if (fds[i].fd == outfd)
@@ -825,11 +849,10 @@ virCommandProcessIO(virCommandPtr cmd)
                 } else {
                     if (VIR_REALLOC_N(*buf, *len + done + 1) < 0) {
                         virReportOOMError();
-                        return -1;
+                        goto cleanup;
                     }
                     memcpy(*buf + *len, data, done);
                     *len += done;
-                    (*buf)[*len] = '\0';
                 }
             } else {
                 int done;
@@ -841,7 +864,7 @@ virCommandProcessIO(virCommandPtr cmd)
                         errno != EAGAIN) {
                         virReportSystemError(errno, "%s",
                                              _("unable to write to child input"));
-                        return -1;
+                        goto cleanup;
                     }
                 } else {
                     inoff += done;
@@ -856,7 +879,13 @@ virCommandProcessIO(virCommandPtr cmd)
         }
     }
 
-    return 0;
+    ret = 0;
+cleanup:
+    if (*cmd->outbuf)
+        (*cmd->outbuf)[outlen] = '\0';
+    if (*cmd->errbuf)
+        (*cmd->errbuf)[errlen] = '\0';
+    return ret;
 }
 
 

diff --git a/tests/commandtest.c b/tests/commandtest.c
index 5971c887e9b09ac9c1161d0ca705baa5708fdf51..e95620500bc113e4093ea0e8563732a54357f993 100644 (file)
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -610,6 +610,63 @@ cleanup:
     return ret;
 }
 
+/*
+ * Test string handling when no output is present.
+ */
+static int test17(const void *unused ATTRIBUTE_UNUSED)
+{
+    virCommandPtr cmd = virCommandNew("/bin/true");
+    int ret = -1;
+    char *outbuf;
+    char *errbuf;
+
+    virCommandSetOutputBuffer(cmd, &outbuf);
+    if (outbuf != NULL) {
+        puts("buffer not sanitized at registration");
+        goto cleanup;
+    }
+
+    if (virCommandRun(cmd, NULL) < 0) {
+        virErrorPtr err = virGetLastError();
+        printf("Cannot run child %s\n", err->message);
+        goto cleanup;
+    }
+
+    if (!outbuf || *outbuf) {
+        puts("output buffer is not an allocated empty string");
+        goto cleanup;
+    }
+    VIR_FREE(outbuf);
+    if ((outbuf = strdup("should not be leaked")) == NULL) {
+        puts("test framework failure");
+        goto cleanup;
+    }
+
+    virCommandSetErrorBuffer(cmd, &errbuf);
+    if (errbuf != NULL) {
+        puts("buffer not sanitized at registration");
+        goto cleanup;
+    }
+
+    if (virCommandRun(cmd, NULL) < 0) {
+        virErrorPtr err = virGetLastError();
+        printf("Cannot run child %s\n", err->message);
+        goto cleanup;
+    }
+
+    if (!outbuf || *outbuf || !errbuf || *errbuf) {
+        puts("output buffers are not allocated empty strings");
+        goto cleanup;
+    }
+
+    ret = 0;
+cleanup:
+    virCommandFree(cmd);
+    VIR_FREE(outbuf);
+    VIR_FREE(errbuf);
+    return ret;
+}
+
 static int
 mymain(int argc, char **argv)
 {
@@ -673,6 +730,7 @@ mymain(int argc, char **argv)
     DO_TEST(test14);
     DO_TEST(test15);
     DO_TEST(test16);
+    DO_TEST(test17);
 
     return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
 }