8:37 p.m.
>> On 3/29/2016 at 09:00 AM, in message
<56F9D3C0.7020907(a)suse.com>, Jim Fehlig
<jfehlig(a)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(a)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