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