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