command: allow merging stdout and stderr in string capture

Sometimes, its easier to run children with 2>&1 in shell notation,
and just deal with stdout and stderr interleaved.  This was already
possible for fd handling; extend it to also work when doing string
capture of a child process.

* docs/internals/command.html.in: Document this.
* src/util/command.c (virCommandSetErrorBuffer): Likewise.
(virCommandRun, virExecWithHook): Implement it.
* tests/commandtest.c (test14): Test it.
* daemon/remote.c (remoteDispatchAuthPolkit): Use new command
feature.

daemon/remote.c

@@ -2477,7 +2477,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED,
     int status = -1;
     char *ident = NULL;
     bool authdismissed = 0;
-    char *pkout = NULL, *pkerr = NULL;
+    char *pkout = NULL;
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
     virCommandPtr cmd = NULL;
@@ -2489,7 +2489,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED,
 
     cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", action, NULL);
     virCommandSetOutputBuffer(cmd, &pkout);
-    virCommandSetErrorBuffer(cmd, &pkerr);
+    virCommandSetErrorBuffer(cmd, &pkout);
 
     VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client));
     if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) {
@@ -2548,17 +2548,12 @@ error:
     if (authdismissed) {
         virNetError(VIR_ERR_AUTH_CANCELLED, "%s",
                     _("authentication cancelled by user"));
-    } else if (pkout || pkerr) {
-        virNetError(VIR_ERR_AUTH_FAILED, "%s %s",
-                    pkerr ? pkerr : "",
-                    pkout ? pkout : "");
     } else {
         virNetError(VIR_ERR_AUTH_FAILED, "%s",
-                    _("authentication failed"));
+                    pkout && *pkout ? pkout : _("authentication failed"));
     }
 
     VIR_FREE(pkout);
-    VIR_FREE(pkerr);
     virNetMessageSaveError(rerr);
     virMutexUnlock(&priv->lock);
     return -1;

docs/internals/command.html.in

@@ -373,7 +373,10 @@
       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.
+      buffers are designed to persist beyond virCommandFree.  It
+      is possible to pass the same pointer to both
+      virCommandSetOutputBuffer and virCommandSetErrorBuffer, in which
+      case the child process interleaves output into a single string.
     </p>
 
     <h3><a name="directory">Setting working directory</a></h3>

src/util/command.c

@@ -464,7 +464,9 @@ virExecWithHook(const char *const*argv,
     }
 
     if (errfd != NULL) {
-        if (*errfd == -1) {
+        if (errfd == outfd) {
+            childerr = childout;
+        } else if (*errfd == -1) {
             if (pipe2(pipeerr, O_CLOEXEC) < 0) {
                 virReportSystemError(errno,
                                      "%s", _("Failed to create pipe"));
@@ -1428,7 +1430,10 @@ virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
  * guaranteed to be allocated after successful virCommandRun or
  * virCommandWait, and is best-effort allocated after failed
  * virCommandRun; caller is responsible for freeing *errbuf.
- * This requires the use of virCommandRun.
+ * This requires the use of virCommandRun.  It is possible to
+ * pass the same pointer as for virCommandSetOutputBuffer(), in
+ * which case the child process will interleave all output into
+ * a single string.
  */
 void
 virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf)
@@ -1940,6 +1945,13 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
         cmd->inpipe = infd[1];
     }
 
+    /* If caller requested the same string for stdout and stderr, then
+     * merge those into one string.  */
+    if (cmd->outbuf && cmd->outbuf == cmd->errbuf) {
+        cmd->errfdptr = &cmd->outfd;
+        cmd->errbuf = NULL;
+    }
+
     /* If caller hasn't requested capture of stdout/err, then capture
      * it ourselves so we can log it.  But the intermediate child for
      * a daemon has no expected output, and we don't want our

tests/commandtest.c

@@ -508,6 +508,14 @@ static int test14(const void *unused ATTRIBUTE_UNUSED)
     const char *errexpect = "BEGIN STDERR\n"
         "Hello World\n"
         "END STDERR\n";
+
+    char *jointactual = NULL;
+    const char *jointexpect = "BEGIN STDOUT\n"
+        "BEGIN STDERR\n"
+        "Hello World\n"
+        "Hello World\n"
+        "END STDOUT\n"
+        "END STDERR\n";
     int ret = -1;
 
     virCommandSetInputBuffer(cmd, "Hello World\n");
@@ -523,7 +531,18 @@ static int test14(const void *unused ATTRIBUTE_UNUSED)
         goto cleanup;
 
     virCommandFree(cmd);
-    cmd = NULL;
+
+    cmd = virCommandNew(abs_builddir "/commandhelper");
+    virCommandSetInputBuffer(cmd, "Hello World\n");
+    virCommandSetOutputBuffer(cmd, &jointactual);
+    virCommandSetErrorBuffer(cmd, &jointactual);
+    if (virCommandRun(cmd, NULL) < 0) {
+        virErrorPtr err = virGetLastError();
+        printf("Cannot run child %s\n", err->message);
+        goto cleanup;
+    }
+    if (!jointactual)
+        goto cleanup;
 
     if (!STREQ(outactual, outexpect)) {
         virtTestDifference(stderr, outexpect, outactual);
@@ -533,6 +552,10 @@ static int test14(const void *unused ATTRIBUTE_UNUSED)
         virtTestDifference(stderr, errexpect, erractual);
         goto cleanup;
     }
+    if (!STREQ(jointactual, jointexpect)) {
+        virtTestDifference(stderr, jointexpect, jointactual);
+        goto cleanup;
+    }
 
     ret = checkoutput("test14");
 
@@ -540,6 +563,7 @@ cleanup:
     virCommandFree(cmd);
     VIR_FREE(outactual);
     VIR_FREE(erractual);
+    VIR_FREE(jointactual);
     return ret;
 }