qemu: use line breaks in command line args written to log

The QEMU command line arguments are very long and currently all written
on a single line to /var/log/libvirt/qemu/$GUEST.log. This introduces
logic to add line breaks after every env variable and "-" optional
argument, and every positional argument. This will create a clearer log
file, which will in turn present better in bug reports when people cut +
paste from the log into a bug comment.

An example log file entry now looks like this:

  2018-12-14 12:57:03.677+0000: starting up libvirt version: 5.0.0, qemu version: 3.0.0qemu-3.0.0-1.fc29, kernel: 4.19.5-300.fc29.x86_64, hostname: localhost.localdomain
  LC_ALL=C \
  PATH=/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin \
  HOME=/home/berrange \
  USER=berrange \
  LOGNAME=berrange \
  QEMU_AUDIO_DRV=none \
  /usr/bin/qemu-system-ppc64 \
  -name guest=guest,debug-threads=on \
  -S \
  -object secret,id=masterKey0,format=raw,file=/home/berrange/.config/libvirt/qemu/lib/domain-33-guest/master-key.aes \
  -machine pseries-2.10,accel=tcg,usb=off,dump-guest-core=off \
  -m 1024 \
  -realtime mlock=off \
  -smp 1,sockets=1,cores=1,threads=1 \
  -uuid c8a74977-ab18-41d0-ae3b-4041c7fffbcd \
  -display none \
  -no-user-config \
  -nodefaults \
  -chardev socket,id=charmonitor,fd=23,server,nowait \
  -mon chardev=charmonitor,id=monitor,mode=control \
  -rtc base=utc \
  -no-shutdown \
  -boot strict=on \
  -device qemu-xhci,id=usb,bus=pci.0,addr=0x1 \
  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
  -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
  -msg timestamp=on
  2018-12-14 12:57:03.730+0000: shutting down, reason=failed

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 43f51a49bb7908afcdfb45aaf110801e996a27a8..8a9061e70f2b8533f6bbf93887ea1ea50f5c7af4 100644 (file)
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -426,7 +426,7 @@ dprintf(logfd, "%s: ", timestamp);
 VIR_FREE(timestamp);
 virCommandWriteArgLog(cmd, logfd);
 
-string = virCommandToString(cmd);
+string = virCommandToString(cmd, false);
 if (string)
     VIR_DEBUG("about to run %s", string);
 VIR_FREE(string);

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 4998100bc2ed5182b355bfd1bcc4ad7490cd582c..912797cfcf8fc4334bc4ec26175e59e572f00cb8 100644 (file)
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -733,14 +733,14 @@ bhyveConnectDomainXMLToNative(virConnectPtr conn,
                                                 NULL)))
             goto cleanup;
 
-        virBufferAdd(&buf, virCommandToString(loadcmd), -1);
+        virBufferAdd(&buf, virCommandToString(loadcmd, false), -1);
         virBufferAddChar(&buf, '\n');
     }
 
     if (!(cmd = virBhyveProcessBuildBhyveCmd(conn, def, true)))
         goto cleanup;
 
-    virBufferAdd(&buf, virCommandToString(cmd), -1);
+    virBufferAdd(&buf, virCommandToString(cmd, false), -1);
 
     if (virBufferCheckError(&buf) < 0)
         goto cleanup;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d1ec112086eae5ff6ace19b16e5934a13adc9dd3..e32257f21eee82081e727c1041317edf9e89a589 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7425,7 +7425,7 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
                                             VIR_QEMU_PROCESS_START_COLD)))
         goto cleanup;
 
-    ret = virCommandToString(cmd);
+    ret = virCommandToString(cmd, false);
 
  cleanup:
     virCommandFree(cmd);

diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 4c066d2ef3aefa7074e93c37c5f561b8342e9bab..2607dea1f5bff6ede4676bd21bd325595f5452d4 100644 (file)
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -360,7 +360,7 @@ qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
         char ebuf[1024];
         char *errstr = NULL;
 
-        if (!(cmdstr = virCommandToString(cmd)))
+        if (!(cmdstr = virCommandToString(cmd, false)))
             goto cleanup;
         virCommandAbort(cmd);
 

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 685e7201e356da04391527e82da62b2f25597631..e5b567228c6dc7e855514782267a475480904cf4 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4353,7 +4353,7 @@ qemuLogOperation(virDomainObjPtr vm,
         goto cleanup;
 
     if (cmd) {
-        char *args = virCommandToString(cmd);
+        char *args = virCommandToString(cmd, true);
         qemuDomainLogContextWrite(logCtxt, "%s\n", args);
         VIR_FREE(args);
     }

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 0d28cae0b76cd013476bd7fa1c703cae1e1049c8..43310361ba6bbccef5dcfde53ee700d4cf51f0df 100644 (file)
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -654,7 +654,7 @@ AppArmorSetSecurityChildProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
     if ((profile_name = get_profile_name(def)) == NULL)
         goto cleanup;
 
-    cmd_str = virCommandToString(cmd);
+    cmd_str = virCommandToString(cmd, false);
     VIR_DEBUG("Changing AppArmor profile to %s on %s", profile_name, cmd_str);
     virCommandSetAppArmorProfile(cmd, profile_name);
     rc = 0;

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 5c170df7b13ead885ca85a4d2b25508ebe297604..d965068369ba3e525da0cb916ea88562e0b2c03a 100644 (file)
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1960,6 +1960,7 @@ virCommandWriteArgLog(virCommandPtr cmd, int logfd)
 /**
  * virCommandToString:
  * @cmd: the command to convert
+ * @linebreaks: true to break line after each env var or option
  *
  * Call after adding all arguments and environment settings, but
  * before Run/RunAsync, to return a string representation of the
@@ -1969,10 +1970,11 @@ virCommandWriteArgLog(virCommandPtr cmd, int logfd)
  * Caller is responsible for freeing the resulting string.
  */
 char *
-virCommandToString(virCommandPtr cmd)
+virCommandToString(virCommandPtr cmd, bool linebreaks)
 {
     size_t i;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
+    bool prevopt = false;
 
     /* Cannot assume virCommandRun will be called; so report the error
      * now.  If virCommandRun is called, it will report the same error. */
@@ -2001,11 +2003,22 @@ virCommandToString(virCommandPtr cmd)
         virBufferAdd(&buf, cmd->env[i], eq - cmd->env[i]);
         virBufferEscapeShell(&buf, eq);
         virBufferAddChar(&buf, ' ');
+        if (linebreaks)
+            virBufferAddLit(&buf, "\\\n");
     }
     virBufferEscapeShell(&buf, cmd->args[0]);
     for (i = 1; i < cmd->nargs; i++) {
         virBufferAddChar(&buf, ' ');
+        if (linebreaks) {
+            /* Line break if this is a --arg or if
+             * the previous arg was a positional option
+             */
+            if (cmd->args[i][0] == '-' ||
+                !prevopt)
+                virBufferAddLit(&buf, "\\\n");
+        }
         virBufferEscapeShell(&buf, cmd->args[i]);
+        prevopt = (cmd->args[i][0] == '-');
     }
 
     if (virBufferCheckError(&buf) < 0)
@@ -2448,7 +2461,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
         goto cleanup;
     }
 
-    str = virCommandToString(cmd);
+    str = virCommandToString(cmd, false);
     if (dryRunBuffer || dryRunCallback) {
         dryRunStatus = 0;
         if (!str) {
@@ -2589,7 +2602,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
         if (exitstatus && (cmd->rawStatus || WIFEXITED(status))) {
             *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status);
         } else if (status) {
-            VIR_AUTOFREE(char *) str = virCommandToString(cmd);
+            VIR_AUTOFREE(char *) str = virCommandToString(cmd, false);
             VIR_AUTOFREE(char *) st = virProcessTranslateStatus(status);
             bool haveErrMsg = cmd->errbuf && *cmd->errbuf && (*cmd->errbuf)[0];
 

diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index f299acc775cfad9b4b5d4b286de1900fdedca031..dbf50418907849e8c95add9e888929605959557d 100644 (file)
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -172,7 +172,7 @@ void virCommandSetPreExecHook(virCommandPtr cmd,
 void virCommandWriteArgLog(virCommandPtr cmd,
                            int logfd);
 
-char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK;
+char *virCommandToString(virCommandPtr cmd, bool linebreaks) ATTRIBUTE_RETURN_CHECK;
 
 int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups) ATTRIBUTE_RETURN_CHECK;
 

diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index c25d2ddfad7c02212b98d020f75a3b03dece11d6..5a0cf95a44a8da12678a859f24ca39d2e2963159 100644 (file)
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -694,7 +694,7 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule,
         if (ignoreErrors) {
             VIR_DEBUG("Ignoring error running command");
         } else {
-            VIR_AUTOFREE(char *) args = virCommandToString(cmd);
+            VIR_AUTOFREE(char *) args = virCommandToString(cmd, false);
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Failed to apply firewall rules %s: %s"),
                            NULLSTR(args), NULLSTR(error));

diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
index 6d5f19e2c68cb6571837fa9d34e59281fb32f207..d1b486fa64d30bda2586c55d9c329227981b661f 100644 (file)
--- a/tests/bhyvexml2argvtest.c
+++ b/tests/bhyvexml2argvtest.c
@@ -68,13 +68,13 @@ static int testCompareXMLToArgvFiles(const char *xml,
         goto out;
     }
 
-    if (!(actualargv = virCommandToString(cmd)))
+    if (!(actualargv = virCommandToString(cmd, false)))
         goto out;
 
     if (actualdm != NULL)
         virTrimSpaces(actualdm, NULL);
 
-    if (!(actualld = virCommandToString(ldcmd)))
+    if (!(actualld = virCommandToString(ldcmd, false)))
         goto out;
 
     if (virTestCompareToFile(actualargv, cmdline) < 0)

diff --git a/tests/commanddata/test26.log b/tests/commanddata/test26.log
new file mode 100644 (file)
index 0000000..db0d424
--- /dev/null
+++ b/tests/commanddata/test26.log
@@ -0,0 +1 @@
+A=B C=D  E true --foo bar --oooh -f --wizz eek eek -w -z -l --mmm flash bang wallop

diff --git a/tests/commandtest.c b/tests/commandtest.c
index 4d9a468db2deba31b2aaa392a71e3f5ed6a764b3..816a70860f11dd7fe9f037605891c9945449ae62 100644 (file)
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -630,7 +630,7 @@ static int test16(const void *unused ATTRIBUTE_UNUSED)
     virCommandAddArg(cmd, "F");
     virCommandAddArg(cmd, "G  H");
 
-    if ((outactual = virCommandToString(cmd)) == NULL) {
+    if ((outactual = virCommandToString(cmd, false)) == NULL) {
         printf("Cannot convert to string: %s\n", virGetLastErrorMessage());
         goto cleanup;
     }
@@ -1135,6 +1135,67 @@ static int test25(const void *unused ATTRIBUTE_UNUSED)
 }
 
 
+/*
+ * Don't run program; rather, log what would be run.
+ */
+static int test26(const void *unused ATTRIBUTE_UNUSED)
+{
+    virCommandPtr cmd = virCommandNew("true");
+    char *outactual = NULL;
+    const char *outexpect =
+        "A=B \\\n"
+        "C='D  E' \\\n"
+        "true \\\n"
+        "--foo bar \\\n"
+        "--oooh \\\n"
+        "-f \\\n"
+        "--wizz 'eek eek' \\\n"
+        "-w \\\n"
+        "-z \\\n"
+        "-l \\\n"
+        "--mmm flash \\\n"
+        "bang \\\n"
+        "wallop";
+
+    int ret = -1;
+    int fd = -1;
+
+    virCommandAddEnvPair(cmd, "A", "B");
+    virCommandAddEnvPair(cmd, "C", "D  E");
+    virCommandAddArgList(cmd, "--foo", "bar", "--oooh", "-f",
+                         "--wizz", "eek eek", "-w", "-z", "-l",
+                         "--mmm", "flash", "bang", "wallop",
+                         NULL);
+
+    if ((outactual = virCommandToString(cmd, true)) == NULL) {
+        printf("Cannot convert to string: %s\n", virGetLastErrorMessage());
+        goto cleanup;
+    }
+    if ((fd = open(abs_builddir "/commandhelper.log",
+                   O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) {
+        printf("Cannot open log file: %s\n", strerror(errno));
+        goto cleanup;
+    }
+    virCommandWriteArgLog(cmd, fd);
+    if (VIR_CLOSE(fd) < 0) {
+        printf("Cannot close log file: %s\n", strerror(errno));
+        goto cleanup;
+    }
+
+    if (STRNEQ(outactual, outexpect)) {
+        virTestDifference(stderr, outexpect, outactual);
+        goto cleanup;
+    }
+
+    ret = checkoutput("test26", NULL);
+
+ cleanup:
+    virCommandFree(cmd);
+    VIR_FORCE_CLOSE(fd);
+    VIR_FREE(outactual);
+    return ret;
+}
+
 static void virCommandThreadWorker(void *opaque)
 {
     virCommandTestDataPtr test = opaque;
@@ -1288,6 +1349,7 @@ mymain(void)
     DO_TEST(test23);
     DO_TEST(test24);
     DO_TEST(test25);
+    DO_TEST(test26);
 
     virMutexLock(&test->lock);
     if (test->running) {

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 287e9d4ea3dc3a0a9e4ef8b6260021f00d47adf3..5812e8543621dded9be416b6b0fa7e36e36d9c85 100644 (file)
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -620,7 +620,7 @@ testCompareXMLToArgv(const void *data)
         goto cleanup;
     }
 
-    if (!(actualargv = virCommandToString(cmd)))
+    if (!(actualargv = virCommandToString(cmd, false)))
         goto cleanup;
 
     if (virTestCompareToFile(actualargv, args) < 0)

diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c
index 196989cb6d7db11a7ba9660505416095918d2277..2f2d40e0277b9e3f79a371e28ad3e8052c8d581f 100644 (file)
--- a/tests/storagepoolxml2argvtest.c
+++ b/tests/storagepoolxml2argvtest.c
@@ -71,7 +71,7 @@ testCompareXMLToArgvFiles(bool shouldFail,
         goto cleanup;
     };
 
-    if (!(actualCmdline = virCommandToString(cmd))) {
+    if (!(actualCmdline = virCommandToString(cmd, false))) {
         VIR_TEST_DEBUG("pool type %d failed to get commandline\n", def->type);
         goto cleanup;
     }

diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c
index 105705f3483c04aff88c319c79f74ced645fdef9..bc2da37410c868b28d1381d6eeae19ebe9348166 100644 (file)
--- a/tests/storagevolxml2argvtest.c
+++ b/tests/storagevolxml2argvtest.c
@@ -104,14 +104,14 @@ testCompareXMLToArgvFiles(bool shouldFail,
         }
 
         if (convertStep != VIR_STORAGE_VOL_ENCRYPT_CONVERT) {
-            if (!(actualCmdline = virCommandToString(cmd)))
+            if (!(actualCmdline = virCommandToString(cmd, false)))
                 goto cleanup;
         } else {
             char *createCmdline = actualCmdline;
             char *cvtCmdline;
             int rc;
 
-            if (!(cvtCmdline = virCommandToString(cmd)))
+            if (!(cvtCmdline = virCommandToString(cmd, false)))
                 goto cleanup;
 
             rc = virAsprintf(&actualCmdline, "%s\n%s",