Re: [libvirt] [PATCH v2 1/2] conf: add xml element devices/pvpanic
On Mon, Dec 02, 2013 at 11:13:33AM +0100, Peter Krempa wrote:
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(a)cn.fujitsu.com>
> ---
> 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)
Thanks, I'll add the missing parts in next version.
>
> 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.
OK.
> + </p>
> + <p>
> + Example: usage of pvpanic configuration
> + </p>
> +<pre>
> + ...
> + <devices>
> + <pvpanic ioport='0x505'/>
> + </devices>
> + ...
> +</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.
OK.
> + }
> +
> + return 0;
> +}
>
> static int
> virDomainRNGDefFormat(virBufferPtr buf,