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

Re: [libvirt] [PATCH] qemu: Fix -chardev udp if parameters are omitted



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/
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 76df0aa..2a48826 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2274,17 +2274,27 @@ qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias,
         virBufferAsprintf(&buf, "stdio,id=char%s", alias);
         break;
 
-    case VIR_DOMAIN_CHR_TYPE_UDP:
+    case VIR_DOMAIN_CHR_TYPE_UDP: {
+        const char *connectHost = dev->data.udp.connectHost;
+        const char *bindHost = dev->data.udp.bindHost;
+        const char *bindService = dev->data.udp.bindService;
+
+        if (connectHost == NULL)
+            connectHost = "";
+        if (bindHost == NULL)
+            bindHost = "";
+        if (bindService == NULL)
+            bindService = "0";
+
         virBufferAsprintf(&buf,
                           "udp,id=char%s,host=%s,port=%s,localaddr=%s,"
                           "localport=%s",
                           alias,
-                          dev->data.udp.connectHost,
+                          connectHost,
                           dev->data.udp.connectService,
-                          dev->data.udp.bindHost,
-                          dev->data.udp.bindService);
+                          bindHost, bindService);
         break;
-
+    }
     case VIR_DOMAIN_CHR_TYPE_TCP:
         telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET;
         virBufferAsprintf(&buf,
@@ -2371,14 +2381,25 @@ qemuBuildChrArgStr(virDomainChrSourceDefPtr dev, const char *prefix)
         virBufferAddLit(&buf, "stdio");
         break;
 
-    case VIR_DOMAIN_CHR_TYPE_UDP:
+    case VIR_DOMAIN_CHR_TYPE_UDP: {
+        const char *connectHost = dev->data.udp.connectHost;
+        const char *bindHost = dev->data.udp.bindHost;
+        const char *bindService  = dev->data.udp.bindService;
+
+        if (connectHost == NULL)
+            connectHost = "";
+        if (bindHost == NULL)
+            bindHost = "";
+        if (bindService == NULL)
+            bindService = "0";
+
         virBufferAsprintf(&buf, "udp:%s:%s %s:%s",
-                          dev->data.udp.connectHost,
+                          connectHost,
                           dev->data.udp.connectService,
-                          dev->data.udp.bindHost,
-                          dev->data.udp.bindService);
+                          bindHost,
+                          bindService);
         break;
-
+    }
     case VIR_DOMAIN_CHR_TYPE_TCP:
         if (dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET) {
             virBufferAsprintf(&buf, "telnet:%s:%s%s",
@@ -5649,13 +5670,12 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr source,
         host2 = svc1 ? strchr(svc1, '@') : NULL;
         svc2 = host2 ? strchr(host2, ':') : NULL;
 
-        if (svc1)
+        if (svc1 && (svc1 != val)) {
             source->data.udp.connectHost = strndup(val, svc1-val);
-        else
-            source->data.udp.connectHost = strdup(val);
 
-        if (!source->data.udp.connectHost)
-            goto no_memory;
+            if (!source->data.udp.connectHost)
+                goto no_memory;
+        }
 
         if (svc1) {
             svc1++;
@@ -5670,19 +5690,21 @@ qemuParseCommandLineChr(virDomainChrSourceDefPtr source,
 
         if (host2) {
             host2++;
-            if (svc2)
+            if (svc2 && (svc2 != host2)) {
                 source->data.udp.bindHost = strndup(host2, svc2-host2);
-            else
-                source->data.udp.bindHost = strdup(host2);
 
-            if (!source->data.udp.bindHost)
-                goto no_memory;
+                if (!source->data.udp.bindHost)
+                    goto no_memory;
+            }
         }
+
         if (svc2) {
             svc2++;
-            source->data.udp.bindService = strdup(svc2);
-            if (!source->data.udp.bindService)
-                goto no_memory;
+            if (STRNEQ(svc2, "0")) {
+                source->data.udp.bindService = strdup(svc2);
+                if (!source->data.udp.bindService)
+                    goto no_memory;
+            }
         }
     } else if (STRPREFIX(val, "tcp:") ||
                STRPREFIX(val, "telnet:")) {
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args
index 362d860..7d1cb67 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-udp-chardev.args
@@ -3,5 +3,7 @@ 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 \
+-device isa-serial,chardev=charserial0,id=serial0 \
+-chardev udp,id=charserial1,host=,port=9999,localaddr=,localport=0 \
+-device isa-serial,chardev=charserial1,id=serial1 -usb -device \
 virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
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..b612e4b 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@:0 -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'/>

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