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

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 ?