[libvirt] [PATCH] qemu: Fix -chardev udp if parameters are omitted
The following XML: <serial type='udp'> <source mode='connect' service='9999'/> </serial> is accepted by domain_conf.c but maps to the qemu command line: -chardev udp,host=127.0.0.1,port=2222,localaddr=(null),localport=(null) qemu can cope with everything omitting except the connection port, which seems to also be the intent of domain_conf validation, so let's not generate bogus command lines for that case. Additionally, tweak the qemu cli parsing to handle omitted host parameters for -serial udp --- src/qemu/qemu_command.c | 48 +++++++++++--------- .../qemuxml2argv-serial-udp-chardev.args | 6 ++- .../qemuxml2argv-serial-udp-chardev.xml | 4 ++ .../qemuxml2argvdata/qemuxml2argv-serial-udp.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-serial-udp.xml | 4 ++ 5 files changed, 39 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c9b9850..231d7c3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2119,14 +2119,17 @@ qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias, break; case VIR_DOMAIN_CHR_TYPE_UDP: - virBufferVSprintf(&buf, - "udp,id=char%s,host=%s,port=%s,localaddr=%s," - "localport=%s", + virBufferVSprintf(&buf, "udp,id=char%s,port=%s", alias, - dev->data.udp.connectHost, - dev->data.udp.connectService, - dev->data.udp.bindHost, - dev->data.udp.bindService); + dev->data.udp.connectService); + + if (dev->data.udp.connectHost) + virBufferVSprintf(&buf, ",host=%s", dev->data.udp.connectHost); + if (dev->data.udp.bindHost) + virBufferVSprintf(&buf, ",localaddr=%s", dev->data.udp.bindHost); + if (dev->data.udp.bindService) + virBufferVSprintf(&buf, ",localport=%s", + dev->data.udp.bindService); break; case VIR_DOMAIN_CHR_TYPE_TCP: @@ -2216,11 +2219,13 @@ qemuBuildChrArgStr(virDomainChrSourceDefPtr dev, const char *prefix) break; case VIR_DOMAIN_CHR_TYPE_UDP: - virBufferVSprintf(&buf, "udp:%s:%s@%s:%s", - dev->data.udp.connectHost, - dev->data.udp.connectService, - dev->data.udp.bindHost, - dev->data.udp.bindService); + virBufferVSprintf(&buf, "udp:%s:%s", + dev->data.udp.connectHost ?: "", + dev->data.udp.connectService); + if (dev->data.udp.bindService) + virBufferVSprintf(&buf, "@%s:%s", + dev->data.udp.bindHost ?: "", + dev->data.udp.bindService); break; case VIR_DOMAIN_CHR_TYPE_TCP: @@ -5302,13 +5307,12 @@ qemuParseCommandLineChr(const char *val) host2 = svc1 ? strchr(svc1, '@') : NULL; svc2 = host2 ? strchr(host2, ':') : NULL; - if (svc1) + if (svc1 && (svc1 != val)) { def->source.data.udp.connectHost = strndup(val, svc1-val); - else - def->source.data.udp.connectHost = strdup(val); - if (!def->source.data.udp.connectHost) - goto no_memory; + if (!def->source.data.udp.connectHost) + goto no_memory; + } if (svc1) { svc1++; @@ -5323,14 +5327,14 @@ qemuParseCommandLineChr(const char *val) if (host2) { host2++; - if (svc2) + if (svc2 && (svc2 != host2)) { def->source.data.udp.bindHost = strndup(host2, svc2-host2); - else - def->source.data.udp.bindHost = strdup(host2); - if (!def->source.data.udp.bindHost) - goto no_memory; + if (!def->source.data.udp.bindHost) + goto no_memory; + } } + if (svc2) { svc2++; def->source.data.udp.bindService = strdup(svc2); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args index 7525110..8c6a6d5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args @@ -2,6 +2,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev socket,\ id=charmonitor,path=/tmp/test-monitor,server,nowait -mon chardev=charmonitor,\ id=monitor,mode=readline -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -chardev \ -udp,id=charserial0,host=127.0.0.1,port=9998,localaddr=127.0.0.1,localport=9999 \ --device isa-serial,chardev=charserial0,id=serial0 -usb -device \ +udp,id=charserial0,port=9998,host=127.0.0.1,localaddr=127.0.0.1,localport=9999 \ +-device isa-serial,chardev=charserial0,id=serial0 \ +-chardev udp,id=charserial1,port=9999 \ +-device isa-serial,chardev=charserial1,id=serial1 -usb -device \ virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.xml index 12622d4..9627c67 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.xml @@ -25,6 +25,10 @@ <source mode='connect' host='127.0.0.1' service='9998'/> <target port='0'/> </serial> + <serial type='udp'> + <source mode='connect' service='9999'/> + <target port='1'/> + </serial> <console type='udp'> <source mode='bind' host='127.0.0.1' service='9999'/> <source mode='connect' host='127.0.0.1' service='9998'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.args index 53c69bc..cf25fe0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.args @@ -1,4 +1,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial \ -udp:127.0.0.1:9998@127.0.0.1:9999 -parallel none -usb +udp:127.0.0.1:9998@127.0.0.1:9999 -serial udp::9999 -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.xml b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.xml index 8697f5a..f606ea4 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp.xml @@ -25,6 +25,10 @@ <source mode='connect' host='127.0.0.1' service='9998'/> <target port='0'/> </serial> + <serial type='udp'> + <source mode='connect' service='9999'/> + <target port='1'/> + </serial> <console type='udp'> <source mode='bind' host='127.0.0.1' service='9999'/> <source mode='connect' host='127.0.0.1' service='9998'/> -- 1.7.4
On 03/22/2011 02:36 PM, Cole Robinson wrote:
The following XML:
<serial type='udp'> <source mode='connect' service='9999'/> </serial>
is accepted by domain_conf.c but maps to the qemu command line:
-chardev udp,host=127.0.0.1,port=2222,localaddr=(null),localport=(null)
qemu can cope with everything omitting except the connection port, which seems to also be the intent of domain_conf validation, so let's not generate bogus command lines for that case.
In the past, I've been told that the qemu command line should be as full-featured as possible - that is, if we know what qemu will be using as a default, then we should explicitly provide that argument rather than relying on the default (it helps us future-proof if not all versions of qemu take the same defaults). I agree that something needs to be done here to avoid the NULL dereference (glibc is too kind by printing "(null)" instead of crashing); but am not sure that making the qemu command line parameters optional is right, if we instead know what qemu does when those parameters are missing. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 22, 2011 at 04:08:10PM -0600, Eric Blake wrote:
On 03/22/2011 02:36 PM, Cole Robinson wrote:
The following XML:
<serial type='udp'> <source mode='connect' service='9999'/> </serial>
is accepted by domain_conf.c but maps to the qemu command line:
-chardev udp,host=127.0.0.1,port=2222,localaddr=(null),localport=(null)
qemu can cope with everything omitting except the connection port, which seems to also be the intent of domain_conf validation, so let's not generate bogus command lines for that case.
In the past, I've been told that the qemu command line should be as full-featured as possible - that is, if we know what qemu will be using as a default, then we should explicitly provide that argument rather than relying on the default (it helps us future-proof if not all versions of qemu take the same defaults).
I agree that something needs to be done here to avoid the NULL dereference (glibc is too kind by printing "(null)" instead of crashing); but am not sure that making the qemu command line parameters optional is right, if we instead know what qemu does when those parameters are missing.
I'm resurecting that thread because we ended up not carrying the patch and I think we should. I looked and this is done in qemu inet_dgram_opts() function of qemu-sockets.c, basically it does: /* lookup local addr */ memset(&ai,0, sizeof(ai)); ai.ai_flags = AI_PASSIVE; ai.ai_family = peer->ai_family; ai.ai_socktype = SOCK_DGRAM; addr = qemu_opt_get(opts, "localaddr"); port = qemu_opt_get(opts, "localport"); if (addr == NULL || strlen(addr) == 0) { addr = NULL; } if (!port || strlen(port) == 0) port = "0"; if (0 != (rc = getaddrinfo(addr, port, &ai, &local))) { fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, gai_strerror(rc)); return -1; } so it relies on the behaviour of getaddrinfo() in that case, and NULL for addr is fine since the service port then default to "0", if we want to force behaviour on QEmu then I think the best is to use, localaddr=,localport=0 since we can't pass "NULL" addr we can pass an empty addr and the qemu behaviour is that it's mapped to NULL, I doubt they would change this. The patch has been rebased, I use temp variables instead as I find this more readable (but slightly more verbose I admit) and keeps the arguments order. There is a small tweak to the original patch on qemuParseCommandLineChr bindService to avoid taking 0 as anything but the default. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/10/2011 01:15 AM, Daniel Veillard wrote:
On Tue, Mar 22, 2011 at 04:08:10PM -0600, Eric Blake wrote:
On 03/22/2011 02:36 PM, Cole Robinson wrote:
The following XML:
<serial type='udp'> <source mode='connect' service='9999'/> </serial>
is accepted by domain_conf.c but maps to the qemu command line:
-chardev udp,host=127.0.0.1,port=2222,localaddr=(null),localport=(null)
The patch has been rebased, I use temp variables instead as I find this more readable (but slightly more verbose I admit) and keeps the arguments order. There is a small tweak to the original patch on qemuParseCommandLineChr bindService to avoid taking 0 as anything but the default.
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Cole Robinson -
Daniel Veillard -
Eric Blake