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

[Jim Fehlig]

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

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