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?