[libvirt] [PATCH V4 6/6] libxlDomainStart: correct cleanup after failure

[Chun Yan Liu]

>>> On 3/29/2016 at 09:00 AM, in message <56F9D3C0.7020907 at suse.com>, Jim Fehlig
<jfehlig at suse.com> wrote: 
> On 03/21/2016 02:11 AM, Chunyan Liu wrote: 
> > Functions like libxlNetworkPrepareDevices, libxlBuildDomainConfig, 
> > virHostdevPrepareDomainDevices will allocate and reserve some 
> > resources. In libxlDomainStart, after calling these functions, 
> > in case of failure, we need to call libxlDomainCleanup to check 
> > and release resoureces. 
> > Besides, since libxlDomainCleanup will call virDomainLockProcessPause, 
> > so we move virDomainLockProcessStart/Resume to earlier stage. 
>  
> I think this cleanup should be done before applying 4/6. It is really 
> independent of support for vf from an SR-IOV pool. 
>  
> > 
> > Signed-off-by: Chunyan Liu <cyliu at suse.com> 
> > --- 
> >  src/libxl/libxl_domain.c | 41 ++++++++++++++++++++++------------------- 
> >  1 file changed, 22 insertions(+), 19 deletions(-) 
> > 
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c 
> > index d11bf3a..6855ce4 100644 
> > --- a/src/libxl/libxl_domain.c 
> > +++ b/src/libxl/libxl_domain.c 
> > @@ -1083,20 +1083,13 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >                                      vm, true) < 0) 
> >          goto cleanup; 
> >   
> > -    if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > -        goto cleanup; 
> > - 
> > -    if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> > -                               cfg->ctx, &d_config) < 0) 
> > -        goto cleanup; 
> > - 
> > -    if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) 
> > -        goto cleanup; 
> > - 
> > -    if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > -                                       vm->def, VIR_HOSTDEV_SP_PCI) < 0) 
> > -        goto cleanup; 
> > - 
> > +    /* libxlNetworkPrepareDevices, libxlBuildDomainConfig, 
> > +     * virHostdevPrepareDomainDevices will allocate and reserve 
> > +     * some resources. In case of failure, need to release 
> > +     * resoureces. This can be done by calling libxlDomainCleanup. 
> > +     * Since libxlDomainCleanup will call virDomainLockProcessPause, 
> > +     * so we move virDomainLockProcessStart/Resume to here. 
> > +     */ 
>  
> I don't think the comment is necessary. Without looking at git history, one 
> wouldn't know where it was moved from :-). 
>  
> >      if (virDomainLockProcessStart(driver->lockManager, 
> >                                    "xen:///system", 
> >                                    vm, 
> > @@ -1111,6 +1104,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >          goto cleanup; 
> >      VIR_FREE(priv->lockState); 
> >   
> > +    if (libxlNetworkPrepareDevices(vm->def) < 0) 
> > +        goto release_dom; 
> > + 
> > +    if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, 
> > +                               cfg->ctx, &d_config) < 0) 
> > +        goto release_dom; 
> > + 
> > +    if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) 
> > +        goto release_dom; 
> > + 
> > +    if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, 
> > +                                       vm->def, VIR_HOSTDEV_SP_PCI) < 0) 
> > +        goto release_dom; 
> > + 
> >      /* Unlock virDomainObj while creating the domain */ 
> >      virObjectUnlock(vm); 
> >   
> > @@ -1194,16 +1201,12 @@ libxlDomainStart(libxlDriverPrivatePtr driver,  
> virDomainObjPtr vm, 
> >   
> >   cleanup_dom: 
> >      ret = -1; 
> > -    if (priv->deathW) { 
> > -        libxl_evdisable_domain_death(cfg->ctx, priv->deathW); 
> > -        priv->deathW = NULL; 
> > -    } 
>  
> Disabling the death event should be done in another patch IMO, after adding  
> the 
> call to libxlDomainCleanup. 
>  
> >      libxlDomainDestroyInternal(driver, vm); 
> >      vm->def->id = -1; 
> >      virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,  
> VIR_DOMAIN_SHUTOFF_FAILED); 
> >   
> >   release_dom: 
> > -    virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState); 
> > +    libxlDomainCleanup(driver, vm); 
>  
> release_dom is not the best name for the label after this change. I've split 
> this patch into 3 patches and posted the result 
>  
> https://www.redhat.com/archives/libvir-list/2016-March/msg01310.html 

Reviewed the split 3 patches, looks good.

Thanks Jim!

>  
> Can you review and comment on that series? As mentioned above, I'd like to  
> get 
> the resource leaks plugged in libxlDomainStart before adding 4/6. 
>  
> Regards, 
> Jim 
>  
>