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

[Hu Tao]

On Wed, Nov 27, 2013 at 11:39:49AM +0100, Peter Krempa wrote:
> On 11/27/13 09:41, Hu Tao wrote:
> > qemu removes the builtin pvpanic device for all qemu versions since 1.7,
> > in order to support <on_crash>, '-device pvpanic' has to be added to
> > qemu command line.
> > 
> > Signed-off-by: Hu Tao <hutao at cn.fujitsu.com>
> > ---
> >  src/qemu/qemu_capabilities.c | 8 ++++++++
> >  src/qemu/qemu_capabilities.h | 2 ++
> >  src/qemu/qemu_command.c      | 4 ++++
> >  3 files changed, 14 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 548b988..7783997 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -243,6 +243,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
> >                "virtio-mmio",
> >                "ich9-intel-hda",
> >                "kvm-pit-lost-tick-policy",
> > +
> > +              "pvpanic", /* 160 */
> >      );
> >  
> >  struct _virQEMUCaps {
> > @@ -1198,6 +1200,9 @@ virQEMUCapsComputeCmdFlags(const char *help,
> >          virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY);
> >      }
> >  
> > +    if (version >= 1005000)
> > +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC);
> > +
> 
> Hard coding the version number is not a good idea. Libvirt uses qmp
> monitor queries to determine supported devices.
> 
> Please add this in the virQEMUCapsObjectTypes structure instead, which
> will do the qmp detection for you.

Thanks for advising, I'll do it in next version.

> 
> >      return 0;
> >  }
> >  
> > @@ -2561,6 +2566,9 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> >      if (qemuCaps->version >= 1006000)
> >          virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> >  
> > +    if (qemuCaps->version >= 1005000)
> > +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_PVPANIC);
> > +
> 
> Same here ... this should be autoprobed by a function using the
> structure above.
> 
> >      if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0)
> >          goto cleanup;
> >      if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
> > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> > index 02d47c6..06d2fac 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ 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_PVPANIC            = 160, /* -device pvpanic */
> > +
> >      QEMU_CAPS_LAST,                   /* this must always be the last item */
> >  };
> >  
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 763417f..b1307a3 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -9588,6 +9588,10 @@ qemuBuildCommandLine(virConnectPtr conn,
> >          goto error;
> >      }
> > 
> > +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PVPANIC)) {
> > +        virCommandAddArgList(cmd, "-device", "pvpanic", NULL);
> > +    }
> > +
> 
> I remember discussions saying that it's NOT a good idea to enable this
> stuff always. As a result, this device is not being added by qemu as you
> described above. Shouldn't we only add this if the user enables
> <on_crash> actions?

Yes we should. When I was writing the patch, I found that def->onCrash
has a default value even when user doesn't set <on_crash>. I must be
reading the code wrong. Any, let me fix it in next version.