[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, &nodes)) < 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, &nodes)) < 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