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

Re: [libvirt] [PATCH v4 2/2] Add support for multiple serial ports into the Xen driver



On 02/25/2011 07:41 AM, Michal Novotny wrote:
> Hi,
> this is the patch to add support for multiple serial ports to the
> libvirt Xen driver. It support both old style (serial = "pty") and
> new style (serial = [ "/dev/ttyS0", "/dev/ttyS1" ]) definition and
> tests for xml2sexpr, sexpr2xml and xmconfig have been added as well.
> 
> Written and tested on RHEL-5 Xen dom0 and working as designed but
> the Xen version have to have patch for RHBZ #614004 but this patch
> is for upstream version of libvirt.

ACK series (with nits), and applied! (after fixing those nits).  Thanks
for bearing with me as we iterated over improvements to this patch.

> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0e68160..6432b74 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5555,7 +5555,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>              for (j = 0 ; j < i ; j++) {
>                  if (def->parallels[j]->target.port > maxport)
>                      maxport = def->parallels[j]->target.port;
> -            }                                            
> +            }

Spurious whitespace change.  I removed the trailing whitespace from
patch 1, to keep 'make syntax-check' bisect-happy.

> @@ -2121,10 +2161,34 @@ xenFormatSxpr(virConnectPtr conn,
>                  virBufferAddLit(&buf, "(parallel none)");
>              }
>              if (def->serials) {
> -                virBufferAddLit(&buf, "(serial ");
> -                if (xenFormatSxprChr(def->serials[0], &buf) < 0)
> -                    goto error;
> -                virBufferAddLit(&buf, ")");
> +                if ((def->nserials > 1) || (def->serials[0]->target.port != 0)) {
> +                    int maxport = -1;
> +                    int j = 0;
> +
> +                    virBufferAddLit(&buf, "(serial (");
> +                    for (i = 0; i < def->nserials; i++)
> +                        if (def->serials[i]->target.port > maxport)
> +                            maxport = def->serials[i]->target.port;
> +
> +                    for (i = 0; i <= maxport; i++) {
> +                        for (j = 0; j < def->nserials; j++) {
> +                            if (def->serials[j]->target.port == i) {
> +                                if (xenFormatSxprChr(def->serials[j], &buf) < 0)
> +                                    goto error;
> +                                if (j < def->nserials - 1)
> +                                    virBufferAddLit(&buf, " ");
> +                                continue;

This continues the inner loop, which is a waste of time since the loop
will no longer find any matches (that is, if def->serials was correctly
populated with no duplicate ports, which we already ensured in patch 1).

> +                            }

You're missing the output "none" right here.  How'd you miss this?
Because your test files weren't consistent...

> +                if (port && STRNEQ(port, "none") &&
> +                    !(chr = xenParseSxprChar(port, NULL)))
> +                    goto cleanup;
> +
> +                if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0)
> +                    goto no_memory;

Oops, this increments nserials even if no serial was parsed, which means
serials[0] is treated as the all-zero data (which happens to be a "null"
device) rather than omitting the device altogether.

> +                for (i = 0; i < def->nserials; i++)
> +                    if (def->serials[i]->target.port > maxport)
> +                        maxport = def->serials[i]->target.port;
> +
> +                for (i = 0; i <= maxport; i++) {
> +                    for (j = 0; j < def->nserials; j++) {
> +                        if (def->serials[j]->target.port == i) {
> +                            if (xenFormatXMSerial(serialVal, def->serials[j]) < 0)
> +                                goto cleanup;
> +                            continue;
> +                        }
> +                    }

Again, missing output of "none".

> @@ -1721,4 +1823,4 @@ cleanup:
>      if (conf)
>          virConfFree(conf);
>      return (NULL);
> -}
> \ No newline at end of file
> +}

Whoops - how'd we do that?  Good thing you fixed it.  I'm surprised that
'make syntax-check' enforces no duplicate newlines at EOF, but missed
out on missing newline.

> +++ b/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg
> @@ -0,0 +1,25 @@
> +disk = [ "phy:/dev/HostVG/XenGuest2,hda,w", "file:/root/boot.iso,hdc:cdrom,r" ]
> +vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ]
> +parallel = "none"
> +serial = [ "null", "/dev/ttyS1" ]

That passes an explicit null device (/dev/null), rather than leaving the
device unattached.  You meant "none".

> diff --git a/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml b/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml
> new file mode 100644
> index 0000000..7c37879
> --- /dev/null
> +++ b/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml
> @@ -0,0 +1,53 @@

> +    <serial type='null'>
> +      <target port='0'/>
> +    </serial>
> +    <serial type='dev'>
> +      <source path='/dev/ttyS1'/>
> +      <target port='1'/>
> +    </serial>

And deleting the <serial type='null'>.

> +++ b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr
> @@ -0,0 +1 @@
...
> \ No newline at end of file

This directory bugs me for it's use of long lines with no newline; but
cleaning that up is a separate patch.

> +++ b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr
> @@ -0,0 +1 @@
> +(...(serial (/dev/ttyS1))...

That only sticks one serial device on port 0.  You missed "none".

You may want to double check one thing, though - when the first serial
port is left unconnected, this patch series instead ties the <console>
device to default to first connected serial device; is this the right
behavior, or do we need a followup patch to adjust how the console
device is handled when there is no serial device on port 0?

Here's what I squashed in:

diff --git i/src/xenxs/xen_sxpr.c w/src/xenxs/xen_sxpr.c
index 520265d..3a412a6 100644
--- i/src/xenxs/xen_sxpr.c
+++ w/src/xenxs/xen_sxpr.c
@@ -2171,15 +2171,22 @@ xenFormatSxpr(virConnectPtr conn,
                             maxport = def->serials[i]->target.port;

                     for (i = 0; i <= maxport; i++) {
+                        virDomainChrDefPtr chr = NULL;
+
+                        if (i)
+                            virBufferAddLit(&buf, " ");
                         for (j = 0; j < def->nserials; j++) {
                             if (def->serials[j]->target.port == i) {
-                                if (xenFormatSxprChr(def->serials[j],
&buf) < 0)
-                                    goto error;
-                                if (j < def->nserials - 1)
-                                    virBufferAddLit(&buf, " ");
-                                continue;
+                                chr = def->serials[j];
+                                break;
                             }
                         }
+                        if (chr) {
+                            if (xenFormatSxprChr(chr, &buf) < 0)
+                                goto error;
+                        } else {
+                            virBufferAddLit(&buf, "none");
+                        }
                     }
                     virBufferAddLit(&buf, "))");
                 }
diff --git i/src/xenxs/xen_xm.c w/src/xenxs/xen_xm.c
index e4499fc..0acd120 100644
--- i/src/xenxs/xen_xm.c
+++ w/src/xenxs/xen_xm.c
@@ -968,6 +968,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
         /* Try to get the list of values to support multiple serial
ports */
         list = virConfGetValue(conf, "serial");
         if (list && list->type == VIR_CONF_LIST) {
+            int portnum = -1;
+
             list = list->list;
             while (list) {
                 char *port = NULL;
@@ -976,17 +978,22 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
                     goto cleanup;

                 port = list->str;
+                portnum++;
+                if (STREQ(port, "none")) {
+                    list = list->next;
+                    continue;
+                }
+
                 if (VIR_ALLOC(chr) < 0)
                     goto no_memory;
-                if (port && STRNEQ(port, "none") &&
-                    !(chr = xenParseSxprChar(port, NULL)))
+                if (!(chr = xenParseSxprChar(port, NULL)))
                     goto cleanup;

                 if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0)
                     goto no_memory;

                 chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
-                chr->target.port = def->nserials;
+                chr->target.port = portnum;

                 def->serials[def->nserials++] = chr;
                 chr = NULL;
@@ -1157,10 +1164,14 @@ static int xenFormatXMSerial(virConfValuePtr list,
     virConfValuePtr val, tmp;
     int ret;

-    ret = xenFormatSxprChr(serial, &buf);
-    if (ret < 0) {
-       virReportOOMError();
-       goto cleanup;
+    if (serial) {
+        ret = xenFormatSxprChr(serial, &buf);
+        if (ret < 0) {
+            virReportOOMError();
+            goto cleanup;
+        }
+    } else {
+        virBufferAddLit(&buf, "none");
     }
     if (virBufferError(&buf)) {
         virReportOOMError();
@@ -1774,13 +1785,15 @@ virConfPtr xenFormatXM(virConnectPtr conn,
                         maxport = def->serials[i]->target.port;

                 for (i = 0; i <= maxport; i++) {
+                    virDomainChrDefPtr chr = NULL;
                     for (j = 0; j < def->nserials; j++) {
                         if (def->serials[j]->target.port == i) {
-                            if (xenFormatXMSerial(serialVal,
def->serials[j]) < 0)
-                                goto cleanup;
-                            continue;
+                            chr = def->serials[j];
+                            break;
                         }
                     }
+                    if (xenFormatXMSerial(serialVal, chr) < 0)
+                        goto cleanup;
                 }

                 if (serialVal->list != NULL) {
diff --git i/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg
w/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg
index 47f0ad6..287e08a 100644
--- i/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg
+++ w/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg
@@ -22,4 +22,4 @@ vncpasswd = "123poi"
 disk = [ "phy:/dev/HostVG/XenGuest2,hda,w",
"file:/root/boot.iso,hdc:cdrom,r" ]
 vif = [
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu"
]
 parallel = "none"
-serial = [ "null", "/dev/ttyS1" ]
+serial = [ "none", "/dev/ttyS1" ]
diff --git i/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml
w/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml
index 7c37879..03549f0 100644
--- i/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml
+++ w/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml
@@ -37,15 +37,13 @@
       <script path='vif-bridge'/>
       <model type='e1000'/>
     </interface>
-    <serial type='null'>
-      <target port='0'/>
-    </serial>
     <serial type='dev'>
       <source path='/dev/ttyS1'/>
       <target port='1'/>
     </serial>
-    <console type='null'>
-      <target type='serial' port='0'/>
+    <console type='dev'>
+      <source path='/dev/ttyS1'/>
+      <target type='serial' port='1'/>
     </console>
     <input type='mouse' bus='ps2'/>
     <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1'
passwd='123poi'/>
diff --git i/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr
w/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr
index 2b33126..f00e69c 100644
--- i/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr
+++ w/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr
@@ -1 +1 @@
-(vm (name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid
'b5d70dd2-75cd-aca5-1776-9660b059d8bc')(on_poweroff 'destroy')(on_reboot
'restart')(on_crash 'restart')(image (hvm (kernel
'/usr/lib/xen/boot/hvmloader')(vcpus 1)(boot c)(cdrom
'/root/boot.iso')(acpi 1)(usb 1)(parallel none)(serial
(/dev/ttyS1))(device_model '/usr/lib64/xen/bin/qemu-dm')(vnc 1)))(device
(vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode 'w')))(device
(vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script
'vif-bridge')(model 'e1000')(type ioemu))))
\ No newline at end of file
+(vm (name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid
'b5d70dd2-75cd-aca5-1776-9660b059d8bc')(on_poweroff 'destroy')(on_reboot
'restart')(on_crash 'restart')(image (hvm (kernel
'/usr/lib/xen/boot/hvmloader')(vcpus 1)(boot c)(cdrom
'/root/boot.iso')(acpi 1)(usb 1)(parallel none)(serial (none
/dev/ttyS1))(device_model '/usr/lib64/xen/bin/qemu-dm')(vnc 1)))(device
(vbd (dev 'ioemu:hda')(uname 'file:/root/foo.img')(mode 'w')))(device
(vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script
'vif-bridge')(model 'e1000')(type ioemu))))
\ No newline at end of file


-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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