[libvirt] [PATCH v4 0/2] Add support for multiple serial ports into the Xen driver
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. Also, this patch is addressing issue described in RHBZ #670789. Differences between v2 and v3: * Fixed serial port handling if we have definition of device in non-zero port number * Added second test for first port undefined (i.e. port value > 0 for just one serial port) This between v3 and v4 (this one): * Traversal lookup to ensure the right order of the serial ports has been implemented * Parsing and finding new available port number for parallel port has been added as well Michal Signed-off-by: Michal Novotny <minovotn@redhat.com> Michal Novotny (2): Fix virDomainChrDefParseTargetXML() port value handling for serial and parallel ports Add support for multiple serial ports into the Xen driver src/conf/domain_conf.c | 24 +++- src/xenxs/xen_sxpr.c | 92 ++++++++++-- src/xenxs/xen_xm.c | 148 +++++++++++++++++--- .../sexpr2xml-fv-serial-dev-2-ports.sexpr | 1 + .../sexpr2xml-fv-serial-dev-2-ports.xml | 53 +++++++ .../sexpr2xml-fv-serial-dev-2nd-port.sexpr | 1 + .../sexpr2xml-fv-serial-dev-2nd-port.xml | 49 +++++++ tests/sexpr2xmltest.c | 2 + .../test-fullvirt-serial-dev-2-ports.cfg | 25 ++++ .../test-fullvirt-serial-dev-2-ports.xml | 55 +++++++ .../test-fullvirt-serial-dev-2nd-port.cfg | 25 ++++ .../test-fullvirt-serial-dev-2nd-port.xml | 53 +++++++ tests/xmconfigtest.c | 2 + .../xml2sexpr-fv-serial-dev-2-ports.sexpr | 1 + .../xml2sexpr-fv-serial-dev-2-ports.xml | 44 ++++++ .../xml2sexpr-fv-serial-dev-2nd-port.sexpr | 1 + .../xml2sexpr-fv-serial-dev-2nd-port.xml | 40 ++++++ tests/xml2sexprtest.c | 2 + 18 files changed, 578 insertions(+), 40 deletions(-) create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.sexpr create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.sexpr create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.cfg create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.xml create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.xml -- 1.7.3.2
Hi, this is the patch to fix the virDomainChrDefParseTargetXML() functionality to parse the target port from XML if available. This is necessary for multiple serial port support which is the second part of this patch. Michal Signed-off-by: Michal Novotny <minovotn@redhat.com> --- src/conf/domain_conf.c | 24 +++++++++++++++++++++--- 1 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b97c1f0..0e68160 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2974,7 +2974,8 @@ virDomainChrDefParseTargetXML(virCapsPtr caps, default: portStr = virXMLPropString(cur, "port"); if (portStr == NULL) { - /* Not required. It will be assigned automatically later */ + /* Set to negative value to indicate we should set it later */ + def->target.port = -1; break; } @@ -2984,6 +2985,7 @@ virDomainChrDefParseTargetXML(virCapsPtr caps, portStr); goto error; } + def->target.port = port; break; } @@ -5547,7 +5549,15 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (!chr) goto error; - chr->target.port = i; + if (chr->target.port == -1) { + int maxport = -1; + int j; + for (j = 0 ; j < i ; j++) { + if (def->parallels[j]->target.port > maxport) + maxport = def->parallels[j]->target.port; + } + chr->target.port = maxport + 1; + } def->parallels[def->nparallels++] = chr; } VIR_FREE(nodes); @@ -5567,7 +5577,15 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (!chr) goto error; - chr->target.port = i; + if (chr->target.port == -1) { + int maxport = -1; + int j; + for (j = 0 ; j < i ; j++) { + if (def->serials[j]->target.port > maxport) + maxport = def->serials[j]->target.port; + } + chr->target.port = maxport + 1; + } def->serials[def->nserials++] = chr; } VIR_FREE(nodes); -- 1.7.3.2
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. Also, this patch is addressing issue described in RHBZ #670789. Differences between v2 and v3: * Fixed serial port handling if we have definition of device in non-zero port number * Added second test for first port undefined (i.e. port value > 0 for just one serial port) This between v3 and v4 (this one): * Traversal lookup to ensure the right order of the serial ports has been implemented * Parsing and finding new available port number for parallel port has been added as well Michal Signed-off-by: Michal Novotny <minovotn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/xenxs/xen_sxpr.c | 92 ++++++++++-- src/xenxs/xen_xm.c | 148 +++++++++++++++++--- .../sexpr2xml-fv-serial-dev-2-ports.sexpr | 1 + .../sexpr2xml-fv-serial-dev-2-ports.xml | 53 +++++++ .../sexpr2xml-fv-serial-dev-2nd-port.sexpr | 1 + .../sexpr2xml-fv-serial-dev-2nd-port.xml | 49 +++++++ tests/sexpr2xmltest.c | 2 + .../test-fullvirt-serial-dev-2-ports.cfg | 25 ++++ .../test-fullvirt-serial-dev-2-ports.xml | 55 +++++++ .../test-fullvirt-serial-dev-2nd-port.cfg | 25 ++++ .../test-fullvirt-serial-dev-2nd-port.xml | 53 +++++++ tests/xmconfigtest.c | 2 + .../xml2sexpr-fv-serial-dev-2-ports.sexpr | 1 + .../xml2sexpr-fv-serial-dev-2-ports.xml | 44 ++++++ .../xml2sexpr-fv-serial-dev-2nd-port.sexpr | 1 + .../xml2sexpr-fv-serial-dev-2nd-port.xml | 40 ++++++ tests/xml2sexprtest.c | 2 + 18 files changed, 558 insertions(+), 38 deletions(-) create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.sexpr create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.sexpr create mode 100644 tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.cfg create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg create mode 100644 tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.xml create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.xml create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.xml 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; - } + } chr->target.port = maxport + 1; } def->parallels[def->nparallels++] = chr; diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index aac2585..520265d 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -177,6 +177,9 @@ xenParseSxprChar(const char *value, if (value[0] == '/') { def->source.type = VIR_DOMAIN_CHR_TYPE_DEV; + def->source.data.file.path = strdup(value); + if (!def->source.data.file.path) + goto no_memory; } else { if ((tmp = strchr(value, ':')) != NULL) { *tmp = '\0'; @@ -1280,18 +1283,55 @@ xenParseSxpr(const struct sexpr *root, /* Character device config */ if (hvm) { - tmp = sexpr_node(root, "domain/image/hvm/serial"); - if (tmp && STRNEQ(tmp, "none")) { - virDomainChrDefPtr chr; - if ((chr = xenParseSxprChar(tmp, tty)) == NULL) - goto error; - if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { - virDomainChrDefFree(chr); - goto no_memory; + const struct sexpr *serial_root; + bool have_multiple_serials = false; + + serial_root = sexpr_lookup(root, "domain/image/hvm/serial"); + if (serial_root) { + const struct sexpr *cur, *node, *cur2; + int ports_skipped = 0; + + for (cur = serial_root; cur->kind == SEXPR_CONS; cur = cur->u.s.cdr) { + node = cur->u.s.car; + + for (cur2 = node; cur2->kind == SEXPR_CONS; cur2 = cur2->u.s.cdr) { + tmp = cur2->u.s.car->u.value; + + if (tmp && STRNEQ(tmp, "none")) { + virDomainChrDefPtr chr; + if ((chr = xenParseSxprChar(tmp, tty)) == NULL) + goto error; + if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + chr->target.port = def->nserials + ports_skipped; + def->serials[def->nserials++] = chr; + } + else + ports_skipped++; + + have_multiple_serials = true; + } } - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; - def->serials[def->nserials++] = chr; } + + if (!have_multiple_serials) { + tmp = sexpr_node(root, "domain/image/hvm/serial"); + if (tmp && STRNEQ(tmp, "none")) { + virDomainChrDefPtr chr; + if ((chr = xenParseSxprChar(tmp, tty)) == NULL) + goto error; + if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + def->serials[def->nserials++] = chr; + } + } + tmp = sexpr_node(root, "domain/image/hvm/parallel"); if (tmp && STRNEQ(tmp, "none")) { virDomainChrDefPtr chr; @@ -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; + } + } + } + virBufferAddLit(&buf, "))"); + } + else { + virBufferAddLit(&buf, "(serial "); + if (xenFormatSxprChr(def->serials[0], &buf) < 0) + goto error; + virBufferAddLit(&buf, ")"); + } } else { virBufferAddLit(&buf, "(serial none)"); } diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index ce590b9..e4499fc 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -965,20 +965,50 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, chr = NULL; } - if (xenXMConfigGetString(conf, "serial", &str, NULL) < 0) - goto cleanup; - if (str && STRNEQ(str, "none") && - !(chr = xenParseSxprChar(str, NULL))) - goto cleanup; + /* Try to get the list of values to support multiple serial ports */ + list = virConfGetValue(conf, "serial"); + if (list && list->type == VIR_CONF_LIST) { + list = list->list; + while (list) { + char *port = NULL; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + goto cleanup; - if (chr) { - if (VIR_ALLOC_N(def->serials, 1) < 0) { - virDomainChrDefFree(chr); - goto no_memory; + port = list->str; + if (VIR_ALLOC(chr) < 0) + goto no_memory; + if (port && STRNEQ(port, "none") && + !(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; + + def->serials[def->nserials++] = chr; + chr = NULL; + + list = list->next; + } + } else { + /* If domain is not using multiple serial ports we parse data old way */ + if (xenXMConfigGetString(conf, "serial", &str, NULL) < 0) + goto cleanup; + if (str && STRNEQ(str, "none") && + !(chr = xenParseSxprChar(str, NULL))) + goto cleanup; + if (chr) { + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + def->serials[0] = chr; + def->nserials++; } - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; - def->serials[0] = chr; - def->nserials++; } } else { if (!(def->console = xenParseSxprChar("pty", NULL))) @@ -1120,6 +1150,45 @@ cleanup: return -1; } +static int xenFormatXMSerial(virConfValuePtr list, + virDomainChrDefPtr serial) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virConfValuePtr val, tmp; + int ret; + + ret = xenFormatSxprChr(serial, &buf); + if (ret < 0) { + virReportOOMError(); + goto cleanup; + } + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + + if (VIR_ALLOC(val) < 0) { + virReportOOMError(); + goto cleanup; + } + + val->type = VIR_CONF_STRING; + val->str = virBufferContentAndReset(&buf); + tmp = list->list; + while (tmp && tmp->next) + tmp = tmp->next; + if (tmp) + tmp->next = val; + else + list->list = val; + + return 0; + +cleanup: + virBufferFreeAndReset(&buf); + return -1; +} + static int xenFormatXMNet(virConnectPtr conn, virConfValuePtr list, virDomainNetDefPtr net, @@ -1678,17 +1747,50 @@ virConfPtr xenFormatXM(virConnectPtr conn, } if (def->nserials) { - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *str; - int ret; + if ((def->nserials == 1) && (def->serials[0]->target.port == 0)) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *str; + int ret; + + ret = xenFormatSxprChr(def->serials[0], &buf); + str = virBufferContentAndReset(&buf); + if (ret == 0) + ret = xenXMConfigSetString(conf, "serial", str); + VIR_FREE(str); + if (ret < 0) + goto no_memory; + } else { + int j = 0; + int maxport = -1; + virConfValuePtr serialVal = NULL; - ret = xenFormatSxprChr(def->serials[0], &buf); - str = virBufferContentAndReset(&buf); - if (ret == 0) - ret = xenXMConfigSetString(conf, "serial", str); - VIR_FREE(str); - if (ret < 0) - goto no_memory; + if (VIR_ALLOC(serialVal) < 0) + goto no_memory; + serialVal->type = VIR_CONF_LIST; + serialVal->list = NULL; + + 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; + } + } + } + + if (serialVal->list != NULL) { + int ret = virConfSetValue(conf, "serial", serialVal); + serialVal = NULL; + if (ret < 0) + goto no_memory; + } + VIR_FREE(serialVal); + } } else { if (xenXMConfigSetString(conf, "serial", "none") < 0) goto no_memory; @@ -1721,4 +1823,4 @@ cleanup: if (conf) virConfFree(conf); return (NULL); -} \ No newline at end of file +} diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.sexpr b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.sexpr new file mode 100644 index 0000000..e709eb0 --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.sexpr @@ -0,0 +1 @@ +(domain (domid 1)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8ff')(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/ttyS0 /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')(type ioemu)))) diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml new file mode 100644 index 0000000..5e085f9 --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2-ports.xml @@ -0,0 +1,53 @@ +<domain type='xen' id='1'> + <name>fvtest</name> + <uuid>b5d70dd2-75cd-aca5-1776-9660b059d8ff</uuid> + <memory>409600</memory> + <currentMemory>409600</currentMemory> + <vcpu>1</vcpu> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib64/xen/bin/qemu-dm</emulator> + <disk type='file' device='disk'> + <driver name='file'/> + <source file='/root/foo.img'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='file'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:1b:b1:47'/> + <source bridge='xenbr0'/> + <script path='vif-bridge'/> + <target dev='vif1.0'/> + </interface> + <serial type='dev'> + <source path='/dev/ttyS0'/> + <target port='0'/> + </serial> + <serial type='dev'> + <source path='/dev/ttyS1'/> + <target port='1'/> + </serial> + <console type='dev'> + <source path='/dev/ttyS0'/> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='5901' autoport='no'/> + </devices> +</domain> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.sexpr b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.sexpr new file mode 100644 index 0000000..3a14cf9 --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.sexpr @@ -0,0 +1 @@ +(domain (domid 1)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8ff')(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')(type ioemu)))) diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml new file mode 100644 index 0000000..5619376 --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-fv-serial-dev-2nd-port.xml @@ -0,0 +1,49 @@ +<domain type='xen' id='1'> + <name>fvtest</name> + <uuid>b5d70dd2-75cd-aca5-1776-9660b059d8ff</uuid> + <memory>409600</memory> + <currentMemory>409600</currentMemory> + <vcpu>1</vcpu> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib64/xen/bin/qemu-dm</emulator> + <disk type='file' device='disk'> + <driver name='file'/> + <source file='/root/foo.img'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='file'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:1b:b1:47'/> + <source bridge='xenbr0'/> + <script path='vif-bridge'/> + <target dev='vif1.0'/> + </interface> + <serial type='dev'> + <source path='/dev/ttyS1'/> + <target port='1'/> + </serial> + <console type='dev'> + <source path='/dev/ttyS1'/> + <target type='serial' port='1'/> + </console> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='5901' autoport='no'/> + </devices> +</domain> diff --git a/tests/sexpr2xmltest.c b/tests/sexpr2xmltest.c index 9995ec4..9f74ece 100644 --- a/tests/sexpr2xmltest.c +++ b/tests/sexpr2xmltest.c @@ -168,6 +168,8 @@ mymain(int argc, char **argv) DO_TEST("fv-serial-null", "fv-serial-null", 1); DO_TEST("fv-serial-file", "fv-serial-file", 1); + DO_TEST("fv-serial-dev-2-ports", "fv-serial-dev-2-ports", 1); + DO_TEST("fv-serial-dev-2nd-port", "fv-serial-dev-2nd-port", 1); DO_TEST("fv-serial-stdio", "fv-serial-stdio", 1); DO_TEST("fv-serial-pty", "fv-serial-pty", 1); DO_TEST("fv-serial-pipe", "fv-serial-pipe", 1); diff --git a/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.cfg b/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.cfg new file mode 100644 index 0000000..86e7998 --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.cfg @@ -0,0 +1,25 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "d" +pae = 1 +acpi = 1 +apic = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +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 = [ "/dev/ttyS0", "/dev/ttyS1" ] diff --git a/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml b/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml new file mode 100644 index 0000000..be5f8ee --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-serial-dev-2-ports.xml @@ -0,0 +1,55 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory>592896</memory> + <currentMemory>403456</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='xenfv'>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='file'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:66:92:9c'/> + <source bridge='xenbr1'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <serial type='dev'> + <source path='/dev/ttyS0'/> + <target port='0'/> + </serial> + <serial type='dev'> + <source path='/dev/ttyS1'/> + <target port='1'/> + </serial> + <console type='dev'> + <source path='/dev/ttyS0'/> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' passwd='123poi'/> + </devices> +</domain> diff --git a/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg b/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg new file mode 100644 index 0000000..47f0ad6 --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-serial-dev-2nd-port.cfg @@ -0,0 +1,25 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "d" +pae = 1 +acpi = 1 +apic = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +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" ] 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 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory>592896</memory> + <currentMemory>403456</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='xenfv'>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='file'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:66:92:9c'/> + <source bridge='xenbr1'/> + <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> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' passwd='123poi'/> + </devices> +</domain> diff --git a/tests/xmconfigtest.c b/tests/xmconfigtest.c index c4c3014..7d418a5 100644 --- a/tests/xmconfigtest.c +++ b/tests/xmconfigtest.c @@ -219,6 +219,8 @@ mymain(int argc, char **argv) DO_TEST("fullvirt-usbtablet", 2); DO_TEST("fullvirt-usbmouse", 2); DO_TEST("fullvirt-serial-file", 2); + DO_TEST("fullvirt-serial-dev-2-ports", 2); + DO_TEST("fullvirt-serial-dev-2nd-port", 2); DO_TEST("fullvirt-serial-null", 2); DO_TEST("fullvirt-serial-pipe", 2); DO_TEST("fullvirt-serial-pty", 2); diff --git a/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr new file mode 100644 index 0000000..2048159 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.sexpr @@ -0,0 +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/ttyS0 /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 diff --git a/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.xml b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.xml new file mode 100644 index 0000000..e5d1817 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2-ports.xml @@ -0,0 +1,44 @@ +<domain type='xen'> + <name>fvtest</name> + <uuid>b5d70dd275cdaca517769660b059d8bc</uuid> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <memory>409600</memory> + <vcpu>1</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/lib64/xen/bin/qemu-dm</emulator> + <interface type='bridge'> + <source bridge='xenbr0'/> + <mac address='00:16:3e:1b:b1:47'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <disk type='file' device='cdrom'> + <source file='/root/boot.iso'/> + <target dev='hdc'/> + <readonly/> + </disk> + <disk type='file'> + <source file='/root/foo.img'/> + <target dev='ioemu:hda'/> + </disk> + <serial type='dev'> + <source path='/dev/ttyS0'/> + <target port='0'/> + </serial> + <serial type='dev'> + <source path='/dev/ttyS1'/> + <target port='1'/> + </serial> + <graphics type='vnc' port='5917' keymap='ja'/> + </devices> +</domain> diff --git a/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr new file mode 100644 index 0000000..2b33126 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.sexpr @@ -0,0 +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 diff --git a/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.xml b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.xml new file mode 100644 index 0000000..a310e5d --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-fv-serial-dev-2nd-port.xml @@ -0,0 +1,40 @@ +<domain type='xen'> + <name>fvtest</name> + <uuid>b5d70dd275cdaca517769660b059d8bc</uuid> + <os> + <type>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <memory>409600</memory> + <vcpu>1</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <features> + <acpi/> + </features> + <devices> + <emulator>/usr/lib64/xen/bin/qemu-dm</emulator> + <interface type='bridge'> + <source bridge='xenbr0'/> + <mac address='00:16:3e:1b:b1:47'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <disk type='file' device='cdrom'> + <source file='/root/boot.iso'/> + <target dev='hdc'/> + <readonly/> + </disk> + <disk type='file'> + <source file='/root/foo.img'/> + <target dev='ioemu:hda'/> + </disk> + <serial type='dev'> + <source path='/dev/ttyS1'/> + <target port='1'/> + </serial> + <graphics type='vnc' port='5917' keymap='ja'/> + </devices> +</domain> diff --git a/tests/xml2sexprtest.c b/tests/xml2sexprtest.c index 0b46faa..cf530b6 100644 --- a/tests/xml2sexprtest.c +++ b/tests/xml2sexprtest.c @@ -149,6 +149,8 @@ mymain(int argc, char **argv) DO_TEST("fv-serial-null", "fv-serial-null", "fvtest", 1); DO_TEST("fv-serial-file", "fv-serial-file", "fvtest", 1); + DO_TEST("fv-serial-dev-2-ports", "fv-serial-dev-2-ports", "fvtest", 1); + DO_TEST("fv-serial-dev-2nd-port", "fv-serial-dev-2nd-port", "fvtest", 1); DO_TEST("fv-serial-stdio", "fv-serial-stdio", "fvtest", 1); DO_TEST("fv-serial-pty", "fv-serial-pty", "fvtest", 1); DO_TEST("fv-serial-pipe", "fv-serial-pipe", "fvtest", 1); -- 1.7.3.2
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
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@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
participants (2)
-
Eric Blake -
Michal Novotny