[libvirt] [PATCH v2 2/2] qemu: add support for -device pvpanic

[Hu Tao]

On Mon, Dec 02, 2013 at 02:38:09PM -0700, Eric Blake wrote:
> On 12/01/2013 11:11 PM, Hu Tao wrote:
> > This patch will add -device pvpanic to qemu command line if user enables
> > pvpanic in domain xml and the qemu version supports pvpanic.
> > 
> > Signed-off-by: Hu Tao <hutao at cn.fujitsu.com>
> > ---
> >  src/qemu/qemu_capabilities.c |  3 +++
> >  src/qemu/qemu_capabilities.h |  2 ++
> >  src/qemu/qemu_command.c      | 10 ++++++++++
> >  3 files changed, 15 insertions(+)
> > 
> 
> In addition to Peter's comments,
> 
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -199,6 +199,8 @@ enum virQEMUCapsFlags {
> >      QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */
> >      QEMU_CAPS_KVM_PIT_TICK_POLICY = 159, /* kvm-pit.lost_tick_policy */
> >  
> > +    QEMU_CAPS_DEVICE_PVPANIC     = 160, /* -device pvpanic */
> 
> Alignment looks odd here.

It aligns as most of the enums do. It doesn't look alike the above two
because their names are too long.

> 
> > +    if (def->pvpanic &&
> > +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PVPANIC)) {
> > +        if (def->pvpanic->ioport > 0) {
> 
> Again, is port 0 a valid port (given that you initialized it to -1)?
> 
> I know we haven't been doing a good job of domxml-from-native, but in
> this particular case, can we also fix the command line parser to turn
> '-device pvpanic' into the appropriate pvpanic device allocation?

In the case of native->xml, I think we'd better to keep port 0 in xml.
I'll fix it.

> 
> > +        } else
> 
> Style.  If you used {} for 'if', you must also use it for 'else' (I'm
> still not sure how to enforce it by syntax-check, but it's on my list of
> things that would be nice to have).

Thanks.