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

[Peter Krempa]

On Wed, Nov 29, 2017 at 21:25:08 -0500, John Ferlan wrote:
> 
> 
> 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.

Yes exactly. That is desired since we need to detect the backing chain
anyways and backingStoreRaw and relPath are necessary in that case.

Also they should be NULL at this point anyways, since only the backing
chain detection code fills them.

> 
> 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?

Umm, good point, since we have the parent there we could extract it
right from it.

Semantically I've done it similarly to virStorageSourceNewFromBackingAbsolute
where the new virStorageSource is constructed only from the string.

In case of the relative relationship, we need to copy some stuff from
the parent and thus I pass it in as well.

> 
> > 
> > -                    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
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171130/41d30784/attachment-0001.sig>