[libvirt] [PATCH 2/3] libxl: fix segfault in libxlReconnectDomain

[Cedric Bosdonnat]

On Wed, 2016-07-27 at 17:40 +0100, Joao Martins wrote:
> > This commit also factorizes code between the error and normal ends.
> Also noticed in the rest of the patches, but I think you forgot to include
> the SoB tag (Signed-off-by).

I never include it and it's not mandatory here in the libvirt community.

> > ---
> >  src/libxl/libxl_driver.c | 28 ++++++++++++++++------------
> >  1 file changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index cb501cf..5008c00 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> > @@ -373,11 +373,13 @@ libxlReconnectDomain(virDomainObjPtr vm,
> >      uint8_t *data = NULL;
> >      virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> >      unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI;
> > +    int ret = -1;
> >  
> >  #ifdef LIBXL_HAVE_PVUSB
> >      hostdev_flags |= VIR_HOSTDEV_SP_USB;
> >  #endif
> >  
> > +    virObjectRef(vm);
> >      virObjectLock(vm);
> >  
> >      libxl_dominfo_init(&d_info);
> > @@ -385,18 +387,18 @@ libxlReconnectDomain(virDomainObjPtr vm,
> >      /* Does domain still exist? */
> >      rc = libxl_domain_info(cfg->ctx, &d_info, vm->def->id);
> >      if (rc == ERROR_INVAL) {
> > -        goto out;
> > +        goto error;
> >      } else if (rc != 0) {
> >          VIR_DEBUG("libxl_domain_info failed (code %d), ignoring domain %d",
> >                    rc, vm->def->id);
> > -        goto out;
> > +        goto error;
> >      }
> >  
> >      /* Is this a domain that was under libvirt control? */
> >      if (libxl_userdata_retrieve(cfg->ctx, vm->def->id,
> >                                  "libvirt-xml", &data, &len)) {
> >          VIR_DEBUG("libxl_userdata_retrieve failed, ignoring domain %d", vm->def->id);
> > -        goto out;
> > +        goto error;
> >      }
> >  
> >      /* Update domid in case it changed (e.g. reboot) while we were gone? */
> > @@ -405,7 +407,7 @@ libxlReconnectDomain(virDomainObjPtr vm,
> >      /* Update hostdev state */
> >      if (virHostdevUpdateActiveDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
> >                                              vm->def, hostdev_flags) < 0)
> > -        goto out;
> > +        goto error;
> >  
> >      if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
> >          driver->inhibitCallback(true, driver->inhibitOpaque);
> > @@ -413,21 +415,23 @@ libxlReconnectDomain(virDomainObjPtr vm,
> >      /* Enable domain death events */
> >      libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW);
> After this patch it will always returns an error. Although
> patch 3 of this series does add the "ret = 0" here. I think it should a part of this
> patch.

Indeed, I failed splitting the change correctly, nice catch!

--
Cedric

> >  
> > + cleanup:
> >      libxl_dominfo_dispose(&d_info);
> >      virObjectUnlock(vm);
> > +    virObjectUnref(vm);
> >      virObjectUnref(cfg);
> > -    return 0;
> > +    return ret;
> >  
> > - out:
> > -    libxl_dominfo_dispose(&d_info);
> > + error:
> >      libxlDomainCleanup(driver, vm);
> > -    if (!vm->persistent)
> > +    if (!vm->persistent) {
> >          virDomainObjListRemoveLocked(driver->domains, vm);
> > -    else
> > -        virObjectUnlock(vm);
> > -    virObjectUnref(cfg);
> >  
> > -    return -1;
> > +        /* virDomainObjListRemoveLocked leaves the object unlocked,
> > +         * lock it again to factorize more code. */
> > +        virObjectLock(vm);
> > +    }
> > +    goto cleanup;
> >  }
> >  
> >  static void
> > 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>