Hi, this is the patch for to support multiple serial ports for Xen driver. The definition for Xen has been introduced in BZ #614004 and this is adding support to libvirt-based tools. The patch was originally written for RHEL-5 and libvirt 0.8.2 (RHEL-5.6) where it has been tested using the virsh command and correct device creation has been confirmed in the xend.log to have the same data for serial ports using both `xm create` and `virsh start` commands. Also, domains with both single and multiple serial ports has been tested to confirm it won't introduce any regression and everything was working fine according to my testing. This patch is the forward-port of RHEL-5 version of the patch. Michal Signed-off-by: Michal Novotny <minovotn(a)redhat.com&gt; --- src/xen/xend_internal.c | 19 ++++++++++++- src/xen/xm_internal.c | 66 +++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 44d5a22..35cdd3c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -5959,8 +5959,23 @@ xenDaemonFormatSxpr(virConnectPtr conn, } if (def->serials) { virBufferAddLit(&buf, "(serial "); - if (xenDaemonFormatSxprChr(def->serials[0], &buf) < 0) - goto error; + /* + * If domain doesn't have multiple serial ports defined we + * keep the old-style formatting not to fail the sexpr tests + */ + if (def->nserials > 1) { + for (i = 0; i < def->nserials; i++) { + virBufferAddLit(&buf, "("); + if (xenDaemonFormatSxprChr(def->serials[i], &buf) < 0) + goto error; + virBufferAddLit(&buf, ")"); + } + } + else { + if (xenDaemonFormatSxprChr(def->serials[0], &buf) < 0) + goto error; + } + virBufferAddLit(&buf, ")"); } else { virBufferAddLit(&buf, "(serial none)"); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index bfb6698..1bb62d7 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1432,20 +1432,64 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { chr = NULL; } - if (xenXMConfigGetString(conf, "serial", &str, NULL) < 0) - goto cleanup; - if (str && STRNEQ(str, "none") && - !(chr = xenDaemonParseSxprChar(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; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + goto skipport; + + port = list->str; + if (VIR_ALLOC(chr) < 0) + goto no_memory; - if (chr) { - if (VIR_ALLOC_N(def->serials, 1) < 0) { + if (!(chr = xenDaemonParseSxprChar(port, NULL))) + goto skipport; + + if (VIR_REALLOC_N(def->serials, def->nserials+1) < 0) + goto no_memory; + + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + chr->type = VIR_DOMAIN_CHR_TYPE_PTY; + + /* Implement device type of serial port when appropriate */ + if (STRPREFIX(port, "/dev")) { + chr->type = VIR_DOMAIN_CHR_TYPE_DEV; + chr->target.port = def->nserials; + chr->data.file.path = strdup(port); + + if (!chr->data.file.path) + goto no_memory; + } + + def->serials[def->nserials++] = chr; + chr = NULL; + + skipport: + list = list->next; virDomainChrDefFree(chr); - goto no_memory; } - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; - def->serials[0] = chr; - def->nserials++; + } + /* If domain is not using multiple serial ports we parse data old way */ + else { + if (xenXMConfigGetString(conf, "serial", &str, NULL) < 0) + goto cleanup; + if (str && STRNEQ(str, "none") && + !(chr = xenDaemonParseSxprChar(str, NULL))) + goto cleanup; + + if (chr) { + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + def->serials[0] = chr; + def->nserials++; + } } } else { if (!(def->console = xenDaemonParseSxprChar("pty", NULL)))

On 01/20/2011 02:53 PM, Daniel P. Berrange wrote: >On Thu, Jan 20, 2011 at 01:20:50PM +0100, Michal Novotny wrote: >>Hi, >>this is the patch for to support multiple serial ports >>for Xen driver. The definition for Xen has been >>introduced in BZ #614004 and this is adding >>support to libvirt-based tools. >> >>The patch was originally written for RHEL-5 and libvirt >>0.8.2 (RHEL-5.6) where it has been tested using >>the virsh command and correct device creation has >>been confirmed in the xend.log to have the same data >>for serial ports using both `xm create` and `virsh >>start` commands. Also, domains with both single and >>multiple serial ports has been tested to confirm >>it won't introduce any regression and everything >>was working fine according to my testing. This patch >>is the forward-port of RHEL-5 version of the patch. >> >>Michal >> >>Signed-off-by: Michal Novotny&lt;minovotn(a)redhat.com&gt; >>--- >> src/xen/xend_internal.c | 19 ++++++++++++- >> src/xen/xm_internal.c | 66 +++++++++++++++++++++++++++++++++++++++-------- >> 3 files changed, 73 insertions(+), 14 deletions(-) >> >>diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c >>index 44d5a22..35cdd3c 100644 >>--- a/src/xen/xend_internal.c >>+++ b/src/xen/xend_internal.c >>@@ -5959,8 +5959,23 @@ xenDaemonFormatSxpr(virConnectPtr conn, >> } >> if (def->serials) { >> virBufferAddLit(&buf, "(serial "); >>- if (xenDaemonFormatSxprChr(def->serials[0],&buf)< 0) >>- goto error; >>+ /* >>+ * If domain doesn't have multiple serial ports defined we >>+ * keep the old-style formatting not to fail the sexpr tests >>+ */ >>+ if (def->nserials> 1) { >>+ for (i = 0; i< def->nserials; i++) { >>+ virBufferAddLit(&buf, "("); >>+ if (xenDaemonFormatSxprChr(def->serials[i],&buf)< 0) >>+ goto error; >>+ virBufferAddLit(&buf, ")"); >>+ } >>+ } >>+ else { >>+ if (xenDaemonFormatSxprChr(def->serials[0],&buf)< 0) >>+ goto error; >>+ } >>+ >> virBufferAddLit(&buf, ")"); >> } else { >> virBufferAddLit(&buf, "(serial none)"); >>diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c >>index bfb6698..1bb62d7 100644 >>--- a/src/xen/xm_internal.c >>+++ b/src/xen/xm_internal.c >>@@ -1432,20 +1432,64 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { >> chr = NULL; >> } >> >>- if (xenXMConfigGetString(conf, "serial",&str, NULL)< 0) >>- goto cleanup; >>- if (str&& STRNEQ(str, "none")&& >>- !(chr = xenDaemonParseSxprChar(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; >>+ >>+ if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) >>+ goto skipport; >>+ >>+ port = list->str; >>+ if (VIR_ALLOC(chr)< 0) >>+ goto no_memory; >> >>- if (chr) { >>- if (VIR_ALLOC_N(def->serials, 1)< 0) { >>+ if (!(chr = xenDaemonParseSxprChar(port, NULL))) >>+ goto skipport; >>+ >>+ if (VIR_REALLOC_N(def->serials, def->nserials+1)< 0) >>+ goto no_memory; >>+ >>+ chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; >>+ chr->type = VIR_DOMAIN_CHR_TYPE_PTY; >>+ >>+ /* Implement device type of serial port when appropriate */ >>+ if (STRPREFIX(port, "/dev")) { >>+ chr->type = VIR_DOMAIN_CHR_TYPE_DEV; >>+ chr->target.port = def->nserials; >>+ chr->data.file.path = strdup(port); >>+ >>+ if (!chr->data.file.path) >>+ goto no_memory; >>+ } >>+ >>+ def->serials[def->nserials++] = chr; >>+ chr = NULL; >>+ >>+ skipport: >>+ list = list->next; >> virDomainChrDefFree(chr); >>- goto no_memory; >> } >>- chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; >>- def->serials[0] = chr; >>- def->nserials++; >>+ } >>+ /* If domain is not using multiple serial ports we parse data old way */ >>+ else { >>+ if (xenXMConfigGetString(conf, "serial",&str, NULL)< 0) >>+ goto cleanup; >>+ if (str&& STRNEQ(str, "none")&& >>+ !(chr = xenDaemonParseSxprChar(str, NULL))) >>+ goto cleanup; >>+ >>+ if (chr) { >>+ if (VIR_ALLOC_N(def->serials, 1)< 0) { >>+ virDomainChrDefFree(chr); >>+ goto no_memory; >>+ } >>+ chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; >>+ def->serials[0] = chr; >>+ def->nserials++; >>+ } >> } >> } else { >> if (!(def->console = xenDaemonParseSxprChar("pty", NULL))) > >Hmm, unless I'm missing something, this patch only seems >todo half the job. It lets you parse XM configs, or generate >SEXPRS, needed to start/create guests. It doesn't let you >parse SEXPRS to query XML, or write XM configs to save an >updated guest config. > >Daniel Good point Daniel. What's the good test for that? Just to issue virsh edit and try to change & save something ?

On 01/20/2011 03:07 PM, Daniel P. Berrange wrote: > On Thu, Jan 20, 2011 at 03:06:11PM +0100, Michal Novotny wrote: >> On 01/20/2011 02:53 PM, Daniel P. Berrange wrote: >>> On Thu, Jan 20, 2011 at 01:20:50PM +0100, Michal Novotny wrote: >>>> Hi, >>>> this is the patch for to support multiple serial ports >>>> for Xen driver. The definition for Xen has been >>>> introduced in BZ #614004 and this is adding >>>> support to libvirt-based tools. >>>> >>>> The patch was originally written for RHEL-5 and libvirt >>>> 0.8.2 (RHEL-5.6) where it has been tested using >>>> the virsh command and correct device creation has >>>> been confirmed in the xend.log to have the same data >>>> for serial ports using both `xm create` and `virsh >>>> start` commands. Also, domains with both single and >>>> multiple serial ports has been tested to confirm >>>> it won't introduce any regression and everything >>>> was working fine according to my testing. This patch >>>> is the forward-port of RHEL-5 version of the patch. >>>> >>>> Michal >>>> >>>> Signed-off-by: Michal Novotny&lt;minovotn(a)redhat.com&gt; >>>> --- >>>> src/xen/xend_internal.c | 19 ++++++++++++- >>>> src/xen/xm_internal.c | 66 >>>> +++++++++++++++++++++++++++++++++++++++-------- >>>> 3 files changed, 73 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c >>>> index 44d5a22..35cdd3c 100644 >>>> --- a/src/xen/xend_internal.c >>>> +++ b/src/xen/xend_internal.c >>>> @@ -5959,8 +5959,23 @@ xenDaemonFormatSxpr(virConnectPtr conn, >>>> } >>>> if (def->serials) { >>>> virBufferAddLit(&buf, "(serial "); >>>> - if >>>> (xenDaemonFormatSxprChr(def->serials[0],&buf)< 0) >>>> - goto error; >>>> + /* >>>> + * If domain doesn't have multiple serial ports >>>> defined we >>>> + * keep the old-style formatting not to fail the >>>> sexpr tests >>>> + */ >>>> + if (def->nserials> 1) { >>>> + for (i = 0; i< def->nserials; i++) { >>>> + virBufferAddLit(&buf, "("); >>>> + if >>>> (xenDaemonFormatSxprChr(def->serials[i],&buf)< 0) >>>> + goto error; >>>> + virBufferAddLit(&buf, ")"); >>>> + } >>>> + } >>>> + else { >>>> + if >>>> (xenDaemonFormatSxprChr(def->serials[0],&buf)< 0) >>>> + goto error; >>>> + } >>>> + >>>> virBufferAddLit(&buf, ")"); >>>> } else { >>>> virBufferAddLit(&buf, "(serial none)"); >>>> diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c >>>> index bfb6698..1bb62d7 100644 >>>> --- a/src/xen/xm_internal.c >>>> +++ b/src/xen/xm_internal.c >>>> @@ -1432,20 +1432,64 @@ xenXMDomainConfigParse(virConnectPtr conn, >>>> virConfPtr conf) { >>>> chr = NULL; >>>> } >>>> >>>> - if (xenXMConfigGetString(conf, "serial",&str, NULL)< 0) >>>> - goto cleanup; >>>> - if (str&& STRNEQ(str, "none")&& >>>> - !(chr = xenDaemonParseSxprChar(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; >>>> + >>>> + if ((list->type != VIR_CONF_STRING) || (list->str >>>> == NULL)) >>>> + goto skipport; >>>> + >>>> + port = list->str; >>>> + if (VIR_ALLOC(chr)< 0) >>>> + goto no_memory; >>>> >>>> - if (chr) { >>>> - if (VIR_ALLOC_N(def->serials, 1)< 0) { >>>> + if (!(chr = xenDaemonParseSxprChar(port, NULL))) >>>> + goto skipport; >>>> + >>>> + if (VIR_REALLOC_N(def->serials, >>>> def->nserials+1)< 0) >>>> + goto no_memory; >>>> + >>>> + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; >>>> + chr->type = VIR_DOMAIN_CHR_TYPE_PTY; >>>> + >>>> + /* Implement device type of serial port when >>>> appropriate */ >>>> + if (STRPREFIX(port, "/dev")) { >>>> + chr->type = VIR_DOMAIN_CHR_TYPE_DEV; >>>> + chr->target.port = def->nserials; >>>> + chr->data.file.path = strdup(port); >>>> + >>>> + if (!chr->data.file.path) >>>> + goto no_memory; >>>> + } >>>> + >>>> + def->serials[def->nserials++] = chr; >>>> + chr = NULL; >>>> + >>>> + skipport: >>>> + list = list->next; >>>> virDomainChrDefFree(chr); >>>> - goto no_memory; >>>> } >>>> - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; >>>> - def->serials[0] = chr; >>>> - def->nserials++; >>>> + } >>>> + /* If domain is not using multiple serial ports we parse >>>> data old way */ >>>> + else { >>>> + if (xenXMConfigGetString(conf, "serial",&str, >>>> NULL)< 0) >>>> + goto cleanup; >>>> + if (str&& STRNEQ(str, "none")&& >>>> + !(chr = xenDaemonParseSxprChar(str, NULL))) >>>> + goto cleanup; >>>> + >>>> + if (chr) { >>>> + if (VIR_ALLOC_N(def->serials, 1)< 0) { >>>> + virDomainChrDefFree(chr); >>>> + goto no_memory; >>>> + } >>>> + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; >>>> + def->serials[0] = chr; >>>> + def->nserials++; >>>> + } >>>> } >>>> } else { >>>> if (!(def->console = xenDaemonParseSxprChar("pty", NULL))) >>> Hmm, unless I'm missing something, this patch only seems >>> todo half the job. It lets you parse XM configs, or generate >>> SEXPRS, needed to start/create guests. It doesn't let you >>> parse SEXPRS to query XML, or write XM configs to save an >>> updated guest config. >>> >>> Daniel >> Good point Daniel. What's the good test for that? Just to issue >> virsh edit and try to change& save something ? > The tests/ directory has xmconfigtest, xensexpr2xmltest and > xenxml2sexprtest which can be used to validate all directions > by giving sample config files > > Daniel Well, those tests (all except xencapstest) passed the testing. Michal