Re: [PATCH v2 1/2] conf: add xml element devices/pvpanic

[Peter Krempa <pkrempa@xxxxxxxxxx>]

On 12/02/13 07:11, Hu Tao wrote:
> This patch adds a new xml element devices/pvpanic to support qemu device
> pvpanic. It can be used to receive guest panic notification.
> 
> Signed-off-by: Hu Tao <hutao@xxxxxxxxxxxxxx>
> ---
>  docs/formatdomain.html.in | 25 +++++++++++++++++
>  src/conf/domain_conf.c    | 68 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h    |  9 +++++++
>  3 files changed, 102 insertions(+)

A few issues I see at first glance:

1) you didn't add ABI compatibility check for the pvpanic device
2) XML->XML tests are missing
3) RNG schemas for the new element are missing
4) XML->qemu commandline tests are missing (in 2/2)


> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 1850a2b..0a72baa 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -5080,6 +5080,31 @@ qemu-kvm -net nic,model=? /dev/null
>      </dd>
>    </dl>
>  
> +    <h4><a name="elementsPvpanic">pvpanic device</a></h4>
> +    <p>
> +      pvpanic device enables libvirt to receive panic notification from a QEMU
> +      guest.
> +      <span class="since">Since 1.3.0, QEMU and KVM only</span>

1.3.0? the since tag is supposed to contain a libvirt version. 1.3.0
will not happen that soon. 1.2.1 is what you are looking for.

> +    </p>
> +    <p>
> +      Example: usage of pvpanic configuration
> +    </p>
> +<pre>
> +  ...
> +  &lt;devices&gt;
> +    &lt;pvpanic ioport='0x505'/&gt;
> +  &lt;/devices&gt;
> +  ...
> +</pre>
> +  <dl>
> +    <dt><code>ioport</code></dt>
> +    <dd>
> +      <p>
> +        ioport used by pvpanic.
> +      </p>
> +    </dd>
> +  </dl>
> +
>      <h3><a name="seclabel">Security label</a></h3>
>  
>      <p>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 140eb80..1b8f66f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

...

> @@ -15715,6 +15768,18 @@ virDomainWatchdogDefFormat(virBufferPtr buf,
>      return 0;
>  }
>  
> +static int virDomainPvpanicDefFormat(virBufferPtr buf,
> +                                     virDomainPvpanicDefPtr def)
> +{
> +    if (def->ioport > 0) {
> +        virBufferAsprintf(buf, "    <pvpanic ioport='%#x'/>\n",
> +                          def->ioport);
> +    } else {
> +        virBufferAsprintf(buf, "    <pvpanic/>\n");

Would break syntax-check. For static strings use virBufferAddLit.

> +    }
> +
> +    return 0;
> +}
>  
>  static int
>  virDomainRNGDefFormat(virBufferPtr buf,

Attachment: signature.asc
Description: OpenPGP digital signature

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