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@xxxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list