Re: [libvirt] [PATCH] Add support for multiple serial ports to Xen driver

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.