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

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(a)redhat.com&gt; 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 >