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@xxxxxxxxxx>, RHCE
Virtualization Team (xen userspace), Red Hat
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list