On Mon, Dec 02, 2013 at 02:34:44PM -0700, Eric Blake wrote:
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(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(+)
In addition to Peter's review:
when sending a series, it helps to include a 0/2 cover letter (git
send-email --cover-letter).
OK, will include it 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>
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?
I'd call it 'panic notification', but it doesn't sound like a device
name. Advise?
> + <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.
OK.
> + 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.
OK.
> +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)?
port 0 means disable the device, so there is no need to add it when port
is 0. But if you'd prefer to let the device handle port itself, then
it's OK to add it in the case.
--
Regards,
Hu Tao