[libvirt] Segfault in virDomainObjListSearchName when listing domains (qemu backend)

[Daniel P. Berrange]

On Mon, Jun 28, 2010 at 07:29:43PM +0200, Guido Winkelmann wrote:
> Am Montag, 28. Juni 2010 schrieben Sie:
> > On Mon, Jun 28, 2010 at 06:06:00PM +0200, Guido Winkelmann wrote:
> > > Another segfault, again after calling list in virsh after a domain failed
> > > to start:
> > 
> > I haven't reproduced the crashes, but try this patch which I think might
> > solve one possible flaw.
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 6ae4e8c..26d935a 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -1178,9 +1178,10 @@ static void qemuHandleMonitorDestroy(qemuMonitorPtr
> >  mon, virDomainObjPtr vm)
> >  {
> >      qemuDomainObjPrivatePtr priv = vm->privateData;
> > -    if (priv->mon == mon)
> > +    if (mon && (priv->mon == mon)) {
> >          priv->mon = NULL;
> > -    virDomainObjUnref(vm);
> > +        virDomainObjUnref(vm);
> > +    }
> >  }
> > 
> >  static qemuMonitorCallbacks monitorCallbacks = {
> > @@ -1212,6 +1213,8 @@ qemuConnectMonitor(struct qemud_driver *driver,
> >  virDomainObjPtr vm) * deleted while the monitor is active */
> >      virDomainObjRef(vm);
> > 
> > +    priv->mon = NULL; /* Explicitly nullify it so destroy callback sees
> >  NULL +                       * if it is invoked during construction */
> >      priv->mon = qemuMonitorOpen(vm,
> >                                  priv->monConfig,
> >                                  priv->monJSON,
> 
> Looks good so far. There's is still a problem with domains just not starting 
> up (somtimes / most of the time) if the host host is under some load, but at 
> least it doesn't seem crash from a simple list --all in virsh anymore.

Actually that patch wasn't very nice, so I've prepared a different one
which should fix the problem in a better way. Separately, I'd like to
know what errors you get when QEMU fails to start ?

Some, but not all, codepaths in the qemuMonitorOpen() method
would trigger the destroy callback. The caller does not expect
this to be invoked if construction fails, only during normal
release of the monitor. This resulted in a possible double-unref
of the virDomainObjPtr, because the caller explicitly unrefs
the virDomainObjPtr  if qemuMonitorOpen() fails

* src/qemu/qemu_monitor.c: Don't invoke destroy callback from
  qemuMonitorOpen() failure paths
---
 src/qemu/qemu_monitor.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f428665..ff613a0 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -671,6 +671,12 @@ qemuMonitorOpen(virDomainObjPtr vm,
     return mon;
 
 cleanup:
+    /* We don't want the 'destroy' callback invoked during
+     * cleanup from construction failure, because that can
+     * give a double-unref on virDomainObjPtr in the caller,
+     * so kill the callbacks now.
+     */
+    mon->cb = NULL;
     qemuMonitorUnlock(mon);
     qemuMonitorClose(mon);
     return NULL;
-- 
1.6.6.1



Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|