[libvirt] [PATCH] conf: Ignore panic device on pSeries.

[Cole Robinson]

On 05/07/2015 12:40 PM, Andrea Bolognani wrote:
> The guest firmware already provides the same functionality, so we can
> just safely drop the <panic/> element from the domain definition.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1182388

<devices><panic> is about a specific device though, which qemu ppc doesn't
support. Even if the firmware provides the logical feature (getting PANICKED
notifications from qemu), it doesn't support the device or any explicit qemu
CLI config, so IMO it's correct to reject <panic>. Apps/users will just have
to take that into account, along with all the other XML differences for x86 vs
ppc64.

An alternative could be the unconditionally add <panic> to ppc64 XML since the
logical feature is always available... but you'd probably have to invent a new
<address> scheme or something

Instead I think it's just a documentation patch.

Another thing from that bug: The error message 'your QEMU is too old to
support pvpanic' isn't accurate for non-x86, so it's probably better to change
it to 'this QEMU binary does not support pvpanic' or similar, maybe look at
existing error messages to find a better example.

Another comment below

> ---
>  src/conf/domain_conf.c                             | 17 +++++++-----
>  .../qemuxml2argv-pseries-panic.args                |  7 +++++
>  .../qemuxml2argv-pseries-panic.xml                 | 30 ++++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  2 ++
>  .../qemuxml2xmlout-pseries-panic.xml               | 29 +++++++++++++++++++++
>  tests/qemuxml2xmltest.c                            |  1 +
>  6 files changed, 80 insertions(+), 6 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-panic.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4cd36a1..a7d4efa 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15478,13 +15478,18 @@ virDomainDefParseXML(xmlDocPtr xml,
>          goto error;
>      }
>      if (n > 0) {
> -        virDomainPanicDefPtr panic =
> -            virDomainPanicDefParseXML(nodes[0]);
> -        if (!panic)
> -            goto error;
> +        /* Ignore the panic device on pSeries, as the guest
> +         * firmware already provides the same functionality */
> +        if (!(ARCH_IS_PPC64(def->os.arch) &&
> +              STRPREFIX(def->os.machine, "pseries"))) {
> +            virDomainPanicDefPtr panic =
> +                virDomainPanicDefParseXML(nodes[0]);
> +            if (!panic)
> +                goto error;
>  
> -        def->panic = panic;
> -        VIR_FREE(nodes);
> +            def->panic = panic;
> +            VIR_FREE(nodes);
> +        }
>      }
> 

FWIW, since this is hypervisor specific, this type of stuff shouldn't be in
the (intended to be) generic domain_conf.c. Check out
qemu_domain.c:qemuDomainDefPostParse instead

- Cole