[libvirt] [PATCH] qemu: use line breaks in command line args written to log

[Daniel P. Berrangé]

On Fri, Dec 14, 2018 at 04:42:12PM +0100, Michal Privoznik wrote:
> On 12/14/18 2:07 PM, Daniel P. Berrangé wrote:
> > 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 at redhat.com>
> > ---
> >  docs/internals/command.html.in   |  2 +-
> >  src/bhyve/bhyve_driver.c         |  4 +-
> >  src/qemu/qemu_driver.c           |  2 +-
> >  src/qemu/qemu_interface.c        |  2 +-
> >  src/qemu/qemu_process.c          |  2 +-
> >  src/security/security_apparmor.c |  2 +-
> >  src/util/vircommand.c            | 19 ++++++++--
> >  src/util/vircommand.h            |  2 +-
> >  src/util/virfirewall.c           |  2 +-
> >  tests/bhyvexml2argvtest.c        |  4 +-
> >  tests/commanddata/test26.log     |  1 +
> >  tests/commandtest.c              | 64 +++++++++++++++++++++++++++++++-
> >  tests/qemuxml2argvtest.c         |  2 +-
> >  tests/storagepoolxml2argvtest.c  |  2 +-
> >  tests/storagevolxml2argvtest.c   |  4 +-
> >  15 files changed, 95 insertions(+), 19 deletions(-)
> >  create mode 100644 tests/commanddata/test26.log
> > 
> 
> 
> > @@ -1991,11 +1993,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] == '-');
> 
> Unfortunately, cmd->args[i] can be NULL here. So I guess this needs to
> look slightly different:

Hmm, what scenario would make that happen ?  I didn't get that
failure in the test suite. 

> 
>     for (i = 1; i < cmd->nargs; i++) {
>         virBufferAddChar(&buf, ' ');
> 
>         if (!cmd->args[i])
>             continue;
> 
>         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] == '-');
>     }
> 
> 
> ACK if you fix this.
> 
> Michal

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|