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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/14/18 4:55 PM, Daniel P. Berrangé wrote:
> 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@xxxxxxxxxx>
>>> ---
>>>  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. 


Are you running against the latest HEAD?

Program received signal SIGSEGV, Segmentation fault.
0x000055555563040b in virCommandToString (cmd=0x555555964b10, linebreaks=false) at util/vircommand.c:2012
2012            prevopt = (cmd->args[i][0] == '-');

virCommandToString 1 $ bt
#0  0x000055555563040b in virCommandToString (cmd=0x555555964b10, linebreaks=false) at util/vircommand.c:2012
#1  0x0000555555587c0c in testCompareXMLToArgvFiles (shouldFail=false, poolxml=0x555555951990 "/home/zippy/work/libvirt/libvirt.git/tests/storagepoolxml2xmlin/pool-dir.xml", volxml=0x5555559582e0 "/home/zippy/work/libvirt/libvirt.git/tests/storagevolxml2xmlin/vol-qcow2-zerocapacity.xml", inputpoolxml=0x0, inputvolxml=0x0, cmdline=0x555555958350 "/home/zippy/work/libvirt/libvirt.git/tests/storagevolxml2argvdata/qcow2-zerocapacity.argv", flags=0, parse_flags=0) at storagevolxml2argvtest.c:107
#2  0x0000555555587fee in testCompareXMLToArgvHelper (data=0x7fffffffd4f0) at storagevolxml2argvtest.c:190
#3  0x0000555555588bc1 in virTestRun (title=0x5555556b4e30 "Storage Vol XML-2-argv qcow2-zerocapacity", body=0x555555587d86 <testCompareXMLToArgvHelper>, data=0x7fffffffd4f0) at testutils.c:176
#4  0x0000555555588508 in mymain () at storagevolxml2argvtest.c:259
#5  0x000055555558ac49 in virTestMain (argc=1, argv=0x7fffffffd798, func=0x555555588056 <mymain>) at testutils.c:1114
#6  0x0000555555588a08 in main (argc=1, argv=0x7fffffffd798) at storagevolxml2argvtest.c:307

virCommandToString 1 $ p cmd->args[i]
$1 = 0x0

virCommandToString 1 $ p i
$2 = 6

This happens because info.path or vol->target.path in virStorageBackendCreateQemuImgCmdFromVol() is NULL. Later, info.path is added to cmd on line 1182:

  virCommandAddArg(cmd, info.path);

One can argue that the bug is in fact that info.path is NULL and it may very well be so, but I think the following should not lead to a crash either:

  cmd = virCommandNew("/bin/bash");
  virCommandAddArg(cmd, NULL);
  virCommanRun(cmd);

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux