Re: [PATCHv4 1/6] conf: add 'model' attribute for panic device with values isa, pseries, hyperv

[Jiri Denemark <jdenemar@xxxxxxxxxx>]

On Fri, Nov 13, 2015 at 20:16:35 +0300, Dmitry Andreev wrote:
> Libvirt already has two types of panic devices - pvpanic and pSeries firmware.
> This patch introduces the 'model' attribute and a new type of panic device.
> 
> 'isa' model is for ISA pvpanic device.
> 'pseries' model is a default value for pSeries guests.
> 'hyperv' model is the new type. It's used for Hyper-V crash.
> 
> Schema and docs are updated for the new attribute.
> ---
>  docs/formatdomain.html.in     | 18 ++++++++++++++++--
>  docs/schemas/domaincommon.rng |  9 +++++++++
>  src/conf/domain_conf.c        | 28 ++++++++++++++++++++++++++--
>  src/conf/domain_conf.h        | 11 +++++++++++
>  4 files changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index c88b032..ef30624 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6152,19 +6152,33 @@ qemu-kvm -net nic,model=? /dev/null
>  <pre>
>    ...
>    &lt;devices&gt;
> -    &lt;panic&gt;
> +    &lt;panic model='isa'&gt;
>        &lt;address type='isa' iobase='0x505'/&gt;
>      &lt;/panic&gt;
>    &lt;/devices&gt;
>    ...
>  </pre>
>    <dl>
> +    <dt><code>model</code></dt>
> +      <dd>
> +        <p>
> +          The optional <code>model</code> attribute specifies what type
> +          of panic device is provided. The panic model used when this attribute
> +          is missing depends on the hypervisor and guest arch.
> +        </p>
> +        <ul>
> +          <li>'isa' &mdash; for ISA pvpanic device</li>
> +          <li>'pseries' &mdash; default and valid only for pSeries guests.</li>
> +          <li>'hyperv' &mdash; for Hyper-V crash CPU feature.
> +            <span class="since">Since 2.5.0, QEMU and KVM only</span></li>

"Since 1.3.0". It refers to the libvirt version which supports this
'hyperv' model rather than QEMU version.

> +        </ul>
> +      </dd>
>      <dt><code>address</code></dt>
>      <dd>
>        <p>
>          address of panic. The default ioport is 0x505. Most users
>          don't need to specify an address, and doing so is forbidden
> -        altogether for pSeries guests.
> +        altogether for pseries and hyperv models.
>        </p>
>      </dd>
>    </dl>
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index eb00444..8339280 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -10204,10 +10211,22 @@ virDomainPanicDefParseXML(xmlNodePtr node)
>      if (virDomainDeviceInfoParseXML(node, NULL, &panic->info, 0) < 0)
>          goto error;
>  
> +    model = virXMLPropString(node, "model");
> +    if (model != NULL &&
> +        (int)(panic->model = virDomainPanicModelTypeFromString(model)) < 0) {

panic->model is declared as int, there's no need to typecast it here.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown panic model '%s'"), model);
> +        goto error;
> +    }
> +
> + cleanup:
> +    VIR_FREE(model);
>      return panic;
> +
>   error:
>      virDomainPanicDefFree(panic);
> -    return NULL;
> +    panic = NULL;
> +    goto cleanup;
>  }
>  
>  /* Parse the XML definition for an input device */
> @@ -20629,8 +20648,13 @@ static int virDomainPanicDefFormat(virBufferPtr buf,
>  {
>      virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
>      int indent = virBufferGetIndent(buf, false);
> +    const char *model = virDomainPanicModelTypeToString(def->model);
> +
> +    if (def->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT)
> +        virBufferAddLit(buf, "<panic");
> +    else
> +        virBufferAsprintf(buf, "<panic model='%s'", model);
>  
> -    virBufferAddLit(buf, "<panic");

I think the following code would be better (esp. in case we need to add
another attribute in the future):

    virBufferAddLit(buf, "<panic");

    if (def->model)
        virBufferAsprintf(buf, " model='%s'",
                          virDomainPanicModelTypeToString(def->model));


>      virBufferAdjustIndent(&childrenBuf, indent + 2);
>      if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0)
>          return -1;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f10b534..be18994 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2045,7 +2045,17 @@ struct _virDomainIdMapDef {
>  };
>  
>  
> +typedef enum {
> +    VIR_DOMAIN_PANIC_MODEL_DEFAULT,
> +    VIR_DOMAIN_PANIC_MODEL_ISA,
> +    VIR_DOMAIN_PANIC_MODEL_PSERIES,
> +    VIR_DOMAIN_PANIC_MODEL_HYPERV,
> +
> +    VIR_DOMAIN_PANIC_MODEL_LAST
> +} virDomainPanicModel;
> +
>  struct _virDomainPanicDef {
> +    int model;

Add /* virDomainPanicModel */ comment at the end of this line.

>      virDomainDeviceInfo info;
>  };
>  
...

Jirka

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