[libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain

[John Ferlan]

On 11/24/2017 07:21 AM, Peter Krempa wrote:
> Split out clearing of the backing chain prior to other code since it
> will be required later and optimize few layers of nested conditions and
> loops.
> ---
>  src/qemu/qemu_domain.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 

The usage of IsBacking and HasBacking is an eye-test... Some thoughts
left below inline - to be sure I'm reading properly and also a hmmm
moment I had while reviewing...

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

I'm OK for freeze since this is part of a series fixing are regression
introduced during the 3.9 release (e.g. patch 3 ref to commmit id
'a693fdba').

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2bda4a726b..0e6ebdc0a8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6151,29 +6151,26 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
> 
> -    if (virStorageSourceHasBacking(src)) {
> -        if (force_probe) {
> -            virStorageSourceBackingStoreClear(src);
> -        } else {
> -            /* skip to the end of the chain */
> -            while (virStorageSourceIsBacking(src)) {
> -                if (report_broken &&
> -                    virStorageFileSupportsAccess(src)) {
> -
> -                    if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0)
> -                        goto cleanup;
> -
> -                    if (virStorageFileAccess(src, F_OK) < 0) {
> -                        virStorageFileReportBrokenChain(errno, src, disk->src);
> -                        virStorageFileDeinit(src);
> -                        goto cleanup;
> -                    }
> +    if (force_probe)
> +        virStorageSourceBackingStoreClear(src);

A "slight variation" in the new code is that for force_probe the Clear
would be tried regardless of whether there is a backingStore and the
src->type could be NONE meaning that the VIR_FREE on src->relPath and
src->backingStoreRaw would happen which is I think expected because we'd
be about to at least fill in backingStoreRaw and relPath is a derivation
of that anyway.

As an aside looking at virStorageSourceNewFromBackingRelative (where
relPath can be filled in) - I'm wondering why parent->backingStoreRaw is
passed as @rel but also STRDUP'd into ret->relPath... IOW: is passing
backingStoreRaw necessary?

> 
> -                    virStorageFileDeinit(src);
> -                }
> -                src = src->backingStore;
> +    /* skip to the end of the chain if there is any */
> +    while (virStorageSourceHasBacking(src)) {
> +        if (report_broken &&
> +            virStorageFileSupportsAccess(src)) {
> +
> +            if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0)
> +                goto cleanup;
> +
> +            if (virStorageFileAccess(src, F_OK) < 0) {
> +                virStorageFileReportBrokenChain(errno, src, disk->src);
> +                virStorageFileDeinit(src);
> +                goto cleanup;
>              }
> +
> +            virStorageFileDeinit(src);
>          }
> +        src = src->backingStore;
>      }
> 
>      /* We skipped to the end of the chain. Skip detection if there's the
>