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

[Eric Blake]

On 12/01/2013 11:11 PM, 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 at 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(+)

In addition to Peter's review:

when sending a series, it helps to include a 0/2 cover letter (git
send-email --cover-letter).

> 
> 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>

pvpanic is a qemu term, but I could see the feasibility of other
hypervisors having a paravirt device with a sole purpose of notifying
the host about panics.  Do we want to come up with a more generic name?

> +  <devices>
> +    <pvpanic ioport='0x505'/>
> +  </devices>
> +  ...
> +</pre>
> +  <dl>
> +    <dt><code>ioport</code></dt>
> +    <dd>
> +      <p>
> +        ioport used by pvpanic.

Probably worth documenting that 0x505 is the default port, and that most
users don't need to specify the ioport.


> +    ioport = virXMLPropString(node, "ioport");
> +    if (!ioport) {
> +        pvpanic->ioport = -1;
> +    } else {
> +        if (virStrToLong_i(ioport, NULL, 0, &pvpanic->ioport) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

VIR_ERR_INTERNAL_ERROR is probably the wrong type, since this is easily
triggered by a user.  I know it's copy and paste from other code, but
these days, VIR_ERR_XML_ERROR is preferred in new code reporting a parse
error.

Should virDomainDeviceType be enhanced to include this device type?  And
if so, you need to modify at least virDomainDeviceDefFree() to handle
the new enum value.

> +static int virDomainPvpanicDefFormat(virBufferPtr buf,
> +                                     virDomainPvpanicDefPtr def)
> +{
> +    if (def->ioport > 0) {

Isn't this an off-by-one if someone explicitly requests port 0 (since
your parser initializes to -1 when left unspecified)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131202/109b77b4/attachment-0001.sig>