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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list