[libvirt] [PATCH v4 2/2] Add support for multiple serial ports into the Xen driver
Michal Novotny
minovotn at redhat.com
Mon Feb 28 09:26:09 UTC 2011
On 02/25/2011 07:39 PM, Eric Blake wrote:
> 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.
Oh, thanks a lot for applying it. I hoped it will get applied and it
finally did so thanks a lot for that :) I'm finally not having this one
in the pending queue :)
>> 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.
>
Oh, I did `make syntax-check` as well and it was not complaining for me
so I don't really know why :(
>> @@ -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).
>
Right, thanks for fixing this when applying.
>> + }
> You're missing the output "none" right here. How'd you miss this?
> Because your test files weren't consistent...
Strange. My test files passed fine but maybe it's caused by not enough
tests or similar.
>> + 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.
Oh, so this is basically "null" device? I didn't know that however as I
said, thanks for fixing this.
>> + 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.
>
I don't know how it got there since I did no change on this hunk but
yeah, it's fixed now.
>> +++ 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".
Ah, ok. Thanks.
>> 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?
Well, this is exactly what I was not sure about so some followup patch
would be good if you, libvirt guys, decide the way to handle this.
Michal
--
Michal Novotny<minovotn at redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat
More information about the libvir-list
mailing list