Re: [libvirt] [PATCHv4 3/6] schema: allow multiple panic devices

[Jiri Denemark]

On Fri, Nov 13, 2015 at 20:16:37 +0300, Dmitry Andreev wrote:
> 'model' attribute was added to a panic device but only one panic
> device is allowed. This patch changes panic device presence
> from 'optional' to 'zeroOrMore'.

I'd remove the "schema: " prefix from the subject since this patch
touches more than just schema.

> ---
>  docs/formatdomain.html.in |  1 +
>  docs/schemas/domaincommon.rng |  4 +--
>  src/conf/domain_conf.c| 73 
> ++-
>  src/conf/domain_conf.h|  4 ++-
>  src/qemu/qemu_command.c   | 50 ++---
>  src/qemu/qemu_domain.c| 21 ++---
>  6 files changed, 83 insertions(+), 70 deletions(-)
> 
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8339280..2f17675 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -16407,23 +16409,20 @@ virDomainDefParseXML(xmlDocPtr xml,
>  VIR_FREE(nodes);
>  
>  /* analysis of the panic devices */
> -def->panic = NULL;
>  if ((n = virXPathNodeSet("./devices/panic", ctxt, )) < 0)
>  goto error;
> -if (n > 1) {
> -virReportError(VIR_ERR_XML_ERROR, "%s",
> -   _("only a single panic device is supported"));
> +if (n && VIR_ALLOC_N(def->panics, n) < 0)
>  goto error;
> -}
> -if (n > 0) {
> +for (i = 0; i < n; i++) {
>  virDomainPanicDefPtr panic =
> -virDomainPanicDefParseXML(nodes[0]);
> +virDomainPanicDefParseXML(nodes[i]);
> +

Extra empty line.

>  if (!panic)
>  goto error;
>  
> -def->panic = panic;
> -VIR_FREE(nodes);
> +def->panics[def->npanics++] = panic;
>  }
> +VIR_FREE(nodes);
>  
>  /* analysis of the shmem devices */
>  if ((n = virXPathNodeSet("./devices/shmem", ctxt, )) < 0)
...
> @@ -18157,16 +18154,6 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
>  goto error;
>  }
>  
> -if (src->panic && dst->panic) {
> -if (!virDomainPanicDefCheckABIStability(src->panic, dst->panic))
> -goto error;
> -} else if (src->panic || dst->panic) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("Either both target and source domains or none of "
> - "them must have PANIC device present"));
> -goto error;
> -}
> -
>  if (src->nmems != dst->nmems) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Target domain memory device count %zu "

Ouch, our code for checking ABI stability of panic devices was rather
disgusting. Your patch seems to make it better, but it would be cool if
you could refactor the code in a separate patch (which would be the
first one in this series), add support for panic->model in the same
patch which introduces it elsewhere, and update the check to work with
multiple panic devices in this patch.

...
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3d2b7a0..19ae5f7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1175,12 +1175,23 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>VIR_DOMAIN_INPUT_BUS_USB) < 0)
>  goto cleanup;
>  
> -if (addPanicDevice && !def->panic) {
> -virDomainPanicDefPtr panic;
> -if (VIR_ALLOC(panic) < 0)
> -goto cleanup;
> +if (addPanicDevice) {
> +size_t j;
> +for (j = 0; j < def->npanics; j++) {
> +if (def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT ||
> +def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_PSERIES)
> +break;
> +}
>  
> -def->panic = panic;
> +if (j == def->npanics) {
> +virDomainPanicDefPtr panic;

Wrong indentation.

> +if (VIR_ALLOC(panic) < 0 ||
> +VIR_APPEND_ELEMENT_COPY(def->panics,
> +def->npanics, panic) < 0) {
> +VIR_FREE(panic);
> +goto cleanup;
> +}
> +}
>  }
>  
>  ret = 0;

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCHv4 3/6] schema: allow multiple panic devices

[Dmitry Andreev]

'model' attribute was added to a panic device but only one panic
device is allowed. This patch changes panic device presence
from 'optional' to 'zeroOrMore'.
---
 docs/formatdomain.html.in |  1 +
 docs/schemas/domaincommon.rng |  4 +--
 src/conf/domain_conf.c| 73 ++-
 src/conf/domain_conf.h|  4 ++-
 src/qemu/qemu_command.c   | 50 ++---
 src/qemu/qemu_domain.c| 21 ++---
 6 files changed, 83 insertions(+), 70 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index ef30624..7a14473 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6152,6 +6152,7 @@ qemu-kvm -net nic,model=? /dev/null
 
   ...
   devices
+panic model='hyperv'/
 panic model='isa'
   address type='isa' iobase='0x505'/
 /panic
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index d67b1bf..8965976 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4044,9 +4044,9 @@
 
   
 
-
+
   
-
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8339280..2f17675 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2538,7 +2538,9 @@ void virDomainDefFree(virDomainDefPtr def)
 
 virDomainTPMDefFree(def->tpm);
 
-virDomainPanicDefFree(def->panic);
+for (i = 0; i < def->npanics; i++)
+virDomainPanicDefFree(def->panics[i]);
+VIR_FREE(def->panics);
 
 VIR_FREE(def->idmap.uidmap);
 VIR_FREE(def->idmap.gidmap);
@@ -3617,10 +3619,10 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
 if (cb(def, , >tpm->info, opaque) < 0)
 return -1;
 }
-if (def->panic) {
-device.type = VIR_DOMAIN_DEVICE_PANIC;
-device.data.panic = def->panic;
-if (cb(def, , >panic->info, opaque) < 0)
+device.type = VIR_DOMAIN_DEVICE_PANIC;
+for (i = 0; i < def->npanics; i++) {
+device.data.panic = def->panics[i];
+if (cb(def, , >panics[i]->info, opaque) < 0)
 return -1;
 }
 
@@ -16407,23 +16409,20 @@ virDomainDefParseXML(xmlDocPtr xml,
 VIR_FREE(nodes);
 
 /* analysis of the panic devices */
-def->panic = NULL;
 if ((n = virXPathNodeSet("./devices/panic", ctxt, )) < 0)
 goto error;
-if (n > 1) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("only a single panic device is supported"));
+if (n && VIR_ALLOC_N(def->panics, n) < 0)
 goto error;
-}
-if (n > 0) {
+for (i = 0; i < n; i++) {
 virDomainPanicDefPtr panic =
-virDomainPanicDefParseXML(nodes[0]);
+virDomainPanicDefParseXML(nodes[i]);
+
 if (!panic)
 goto error;
 
-def->panic = panic;
-VIR_FREE(nodes);
+def->panics[def->npanics++] = panic;
 }
+VIR_FREE(nodes);
 
 /* analysis of the shmem devices */
 if ((n = virXPathNodeSet("./devices/shmem", ctxt, )) < 0)
@@ -17633,17 +17632,14 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
src,
 }
 
 static bool
-virDomainPanicCheckABIStability(virDomainPanicDefPtr src,
+virDomainPanicDefCheckABIStability(virDomainPanicDefPtr src,
 virDomainPanicDefPtr dst)
 {
-if (!src && !dst)
-return true;
-
-if (!src || !dst) {
+if (src->model != dst->model) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Target domain panic device count '%d' "
- "does not match source count '%d'"),
-   src ? 1 : 0, dst ? 1 : 0);
+   _("Target panic model '%s' does not match source '%s'"),
+   virDomainPanicModelTypeToString(dst->model),
+   virDomainPanicModelTypeToString(src->model));
 return false;
 }
 
@@ -17709,13 +17705,6 @@ virDomainTPMDefCheckABIStability(virDomainTPMDefPtr 
src,
 }
 
 static bool
-virDomainPanicDefCheckABIStability(virDomainPanicDefPtr src,
-   virDomainPanicDefPtr dst)
-{
-return virDomainDeviceInfoCheckABIStability(>info, >info);
-}
-
-static bool
 virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
 virDomainMemoryDefPtr dst)
 {
@@ -18132,8 +18121,16 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
 if (!virDomainRNGDefCheckABIStability(src->rngs[i], dst->rngs[i]))
 goto error;
 
-if (!virDomainPanicCheckABIStability(src->panic, dst->panic))
+if (src->npanics != dst->npanics) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target domain panic device count %zu "
+ "does not match source %zu"), dst->npanics, 
src->npanics);
 goto error;
+}
+
+for (i = 0; i < src->npanics; i++)
+