[libvirt] [PATCH] Add support for multiple serial ports to Xen driver
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@redhat.com> --- 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))) -- 1.7.3.2
On 01/20/2011 12:58 PM, root wrote:
+ if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + goto skipport; +
I believe this should do xenXMError(VIR_ERR_INTERNAL_ERROR, _("config value %s was malformed"), name); goto cleanup; similar to xenXMConfigGetString.
+ 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; + }
Can you explain what's going on here ("it needs to be this way to fix xyz" is not an explanation; please give the input and output of xenDaemonParseSxprChar). In particular, these look like preexisting problems handling serial = "/dev/ttyS0" (where the port is not copied to chr->data.file.path). Is this correct? If so, please state this explicitly and check that the above works. It seems to me that your patch fixes /dev/ttyS0 in the array case, but not when it is the only serial port. If this is correct, "chr->data.file.path = strdup(port);" should be moved to xenDaemonParseSxprChar. In fact there is already code there to do exactly the same strdup for VIR_DOMAIN_CHR_TYPE_PTY. Even taking the above into consideration, the code is going through some extra hoops (and has a few bugs): 1) chr->type should already be correct if the port starts with "/dev", but you have just overridden it to VIR_DOMAIN_CHR_TYPE_PTY. 2) "chr->type = VIR_DOMAIN_CHR_TYPE_PTY" is not necessary. 3) "chr->target.port = def->nserials;" should be done always, not just when /dev is matched. In the end the code here should be simply: /* Not really necessary (the enum evaluates to 0), but I prefer to have it for clarity. */ chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; chr->target.port = def->nserials; matching more or less the code you have in the "else" branch below. Anything except the above should go in xenDaemonParseSxprChar. Also, please add testcases. There are many cases that should be covered for both XML->sxpr and xm->XML conversion. I initially thought there was another problem in the code, which is what to do when the <target> subelement is present and ends up defining a third serial port without the first being present. However I changed my mind since the qemu driver is anyway disregarding the port number as well. Given this, the patch is already in pretty good shape. Thanks, Paolo
On 01/20/2011 02:32 PM, Paolo Bonzini wrote:
On 01/20/2011 12:58 PM, root wrote:
+ if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + goto skipport; +
I believe this should do
xenXMError(VIR_ERR_INTERNAL_ERROR, _("config value %s was malformed"), name); goto cleanup;
similar to xenXMConfigGetString.
+ 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; + }
Can you explain what's going on here ("it needs to be this way to fix xyz" is not an explanation; please give the input and output of xenDaemonParseSxprChar). In particular, these look like preexisting problems handling
serial = "/dev/ttyS0"
(where the port is not copied to chr->data.file.path). Is this correct? If so, please state this explicitly and check that the above works. It seems to me that your patch fixes /dev/ttyS0 in the array case, but not when it is the only serial port. If this is correct, "chr->data.file.path = strdup(port);" should be moved to xenDaemonParseSxprChar. In fact there is already code there to do exactly the same strdup for VIR_DOMAIN_CHR_TYPE_PTY.
Even taking the above into consideration, the code is going through some extra hoops (and has a few bugs):
1) chr->type should already be correct if the port starts with "/dev", but you have just overridden it to VIR_DOMAIN_CHR_TYPE_PTY.
2) "chr->type = VIR_DOMAIN_CHR_TYPE_PTY" is not necessary.
3) "chr->target.port = def->nserials;" should be done always, not just when /dev is matched.
In the end the code here should be simply:
/* Not really necessary (the enum evaluates to 0), but I prefer to have it for clarity. */ chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; chr->target.port = def->nserials;
matching more or less the code you have in the "else" branch below. Anything except the above should go in xenDaemonParseSxprChar.
Also, please add testcases. There are many cases that should be covered for both XML->sxpr and xm->XML conversion.
I initially thought there was another problem in the code, which is what to do when the <target> subelement is present and ends up defining a third serial port without the first being present. However I changed my mind since the qemu driver is anyway disregarding the port number as well. Given this, the patch is already in pretty good shape.
Thanks,
Paolo
Well, I'm working on support for serial port configuration as serial = [ "/dev/null", "/dev/ttyS0", "/dev/ttyS1" ] to: 1) pass /dev/null to the guest as /dev/ttyS0 2) pass /dev/ttyS0 to the guest as /dev/ttyS1 3) pass /dev/ttyS1 to the guest as /dev/ttyS2 I think this could be good thing to be implemented as well. Also, the libvirt XML definition should be: <serial type='dev'> <source path='/dev/null'/> <target port='0'/> </serial> <serial type='dev'> <source path='/dev/ttyS0'/> <target port='1'/> </serial> <serial type='dev'> <source path='/dev/ttyS1'/> <target port='2'/> </serial> Are you OK with that? I'm also thinking of omitting the first <serial> block and automatically set the path to /dev/null when target.port is not present in the libvirt XML definition but it shouldn't be necessary after all. What do you think on this one? Thanks, Michal -- Michal Novotny<minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On 01/20/2011 02:39 PM, Michal Novotny wrote:
Well, I'm working on support for serial port configuration as serial = [ "/dev/null", "/dev/ttyS0", "/dev/ttyS1" ] to:
1) pass /dev/null to the guest as /dev/ttyS0 2) pass /dev/ttyS0 to the guest as /dev/ttyS1 3) pass /dev/ttyS1 to the guest as /dev/ttyS2
I think this could be good thing to be implemented as well. Also, the libvirt XML definition should be:
<serial type='dev'> <source path='/dev/null'/> <target port='0'/> </serial> <serial type='dev'> <source path='/dev/ttyS0'/> <target port='1'/> </serial> <serial type='dev'> <source path='/dev/ttyS1'/> <target port='2'/> </serial>
Are you OK with that?
Yes, and it should "just work" with your patch.
I'm also thinking of omitting the first <serial> block and automatically set the path to /dev/null when target.port is not present in the libvirt XML definition but it shouldn't be necessary after all. What do you think on this one?
No, even the qemu driver disregards target.port in the XML you give it, as far as I understand. Paolo
Good, I spend many hours on this but I see I'll continue working on this tomorrow since it's not OK yet. Michal ----- Original Message ----- From: "Paolo Bonzini" <pbonzini@redhat.com> To: "Michal Novotny" <minovotn@redhat.com> Cc: "Libvirt" <libvir-list@redhat.com> Sent: Thursday, January 20, 2011 3:13:22 PM Subject: Re: [libvirt] [PATCH] Add support for multiple serial ports to Xen driver On 01/20/2011 02:39 PM, Michal Novotny wrote:
Well, I'm working on support for serial port configuration as serial = [ "/dev/null", "/dev/ttyS0", "/dev/ttyS1" ] to:
1) pass /dev/null to the guest as /dev/ttyS0 2) pass /dev/ttyS0 to the guest as /dev/ttyS1 3) pass /dev/ttyS1 to the guest as /dev/ttyS2
I think this could be good thing to be implemented as well. Also, the libvirt XML definition should be:
<serial type='dev'> <source path='/dev/null'/> <target port='0'/> </serial> <serial type='dev'> <source path='/dev/ttyS0'/> <target port='1'/> </serial> <serial type='dev'> <source path='/dev/ttyS1'/> <target port='2'/> </serial>
Are you OK with that?
Yes, and it should "just work" with your patch.
I'm also thinking of omitting the first <serial> block and automatically set the path to /dev/null when target.port is not present in the libvirt XML definition but it shouldn't be necessary after all. What do you think on this one?
No, even the qemu driver disregards target.port in the XML you give it, as far as I understand. Paolo
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 <minovotn@redhat.com> --- 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
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<minovotn@redhat.com> --- 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 ?
Michal -- Michal Novotny<minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
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<minovotn@redhat.com> --- 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
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:
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@redhat.com> --- 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
On Thu, Jan 20, 2011 at 01:20:50PM +0100, Michal Novotny wrote: 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 -- Michal Novotny<minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On 01/20/2011 03:07 PM, Michal Novotny wrote:
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:
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@redhat.com> --- 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
On Thu, Jan 20, 2011 at 01:20:50PM +0100, Michal Novotny wrote: 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
Just one more thing to be added to the xencapstest. I've been writing and recompiling it on RHEL-5.6 host to test whether it's working fine and this was failing on sysinterface call always - even when right downloaded from CVS. Michal -- Michal Novotny<minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On Thu, Jan 20, 2011 at 03:07:55PM +0100, Michal Novotny wrote:
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:
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@redhat.com> --- 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
On Thu, Jan 20, 2011 at 01:20:50PM +0100, Michal Novotny wrote: 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.
That would be because you didn't add any more config files to actually test the multiple serial port style configs.... Daniel
participants (3)
-
Daniel P. Berrange -
Michal Novotny -
Paolo Bonzini