>>> On 3/29/2016 at 09:00 AM, in message <56F9D3C0.7020907@xxxxxxxx>, Jim Fehlig <jfehlig@xxxxxxxx> 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@xxxxxxxx> > > --- > > 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 > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list