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@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).
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list