[libvirt] [PATCH 0/3] qemu: Yet another backingStore terminator fix (blockdev-add saga)
Raw images would not get the <backingStore/> terminator due to the changes. Peter Krempa (3): qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain qemu: process: Move handling of non-backing files into qemuDomainDetermineDiskChain qemu: domain: Fix backing store terminator for non-backing local files src/qemu/qemu_domain.c | 59 ++++++++++++++-------- src/qemu/qemu_process.c | 10 ---- .../qemuhotplug-base-ccw-live+ccw-virtio.xml | 1 + ...live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 2 + ...qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 1 + ...-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 1 + ...-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 1 + ...-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml | 1 + .../qemuhotplug-base-live+disk-scsi.xml | 1 + .../qemuhotplug-base-live+disk-usb.xml | 1 + .../qemuhotplug-base-live+disk-virtio.xml | 1 + ...se-without-scsi-controller-live+disk-scsi-2.xml | 1 + 12 files changed, 50 insertions(+), 30 deletions(-) -- 2.14.3
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(-) 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); - 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 -- 2.14.3
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
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@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
Until now we would skip loading of the backing chain for files which don't support backing chains only when starting up the VM. Move the check from qemuProcessPrepareHostStorage with some adaptations so that's always applied. --- src/qemu/qemu_domain.c | 17 +++++++++++++++++ src/qemu/qemu_process.c | 10 ---------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0e6ebdc0a8..0cdcb11c37 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6154,6 +6154,23 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, if (force_probe) virStorageSourceBackingStoreClear(src); + /* There is no need to check the backing chain for disks without backing + * support */ + if (virStorageSourceIsLocalStorage(src) && + src->format > VIR_STORAGE_FILE_NONE && + src->format < VIR_STORAGE_FILE_BACKING) { + + if (!virFileExists(src->path)) { + if (report_broken) + virStorageFileReportBrokenChain(errno, src, disk->src); + + goto cleanup; + } + + ret = 0; + goto cleanup; + } + /* skip to the end of the chain if there is any */ while (virStorageSourceHasBacking(src)) { if (report_broken && diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8574f2b413..ea70885dd9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5555,20 +5555,10 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, for (i = vm->def->ndisks; i > 0; i--) { size_t idx = i - 1; virDomainDiskDefPtr disk = vm->def->disks[idx]; - virStorageFileFormat format = virDomainDiskGetFormat(disk); if (virStorageSourceIsEmpty(disk->src)) continue; - /* There is no need to check the backing chain for disks - * without backing support, the fact that the file exists is - * more than enough */ - if (virStorageSourceIsLocalStorage(disk->src) && - format > VIR_STORAGE_FILE_NONE && - format < VIR_STORAGE_FILE_BACKING && - virFileExists(virDomainDiskGetSource(disk))) - continue; - if (qemuDomainDetermineDiskChain(driver, vm, disk, true, true) >= 0) continue; -- 2.14.3
On 11/24/2017 07:21 AM, Peter Krempa wrote:
Until now we would skip loading of the backing chain for files which don't support backing chains only when starting up the VM. Move the check from qemuProcessPrepareHostStorage with some adaptations so that's always applied. --- src/qemu/qemu_domain.c | 17 +++++++++++++++++ src/qemu/qemu_process.c | 10 ---------- 2 files changed, 17 insertions(+), 10 deletions(-)
Does it matter or should the check go before the virStorageSourceBackingStoreClear... Up through this point we did it before anyway. Reviewed-by: John Ferlan <jferlan@redhat.com> (for the code regardless of your decision on placement)
On Wed, Nov 29, 2017 at 21:25:45 -0500, John Ferlan wrote:
On 11/24/2017 07:21 AM, Peter Krempa wrote:
Until now we would skip loading of the backing chain for files which don't support backing chains only when starting up the VM. Move the check from qemuProcessPrepareHostStorage with some adaptations so that's always applied. --- src/qemu/qemu_domain.c | 17 +++++++++++++++++ src/qemu/qemu_process.c | 10 ---------- 2 files changed, 17 insertions(+), 10 deletions(-)
Does it matter or should the check go before the virStorageSourceBackingStoreClear... Up through this point we did it before anyway.
Well. It does not matter at this precise point. If the file does not exist (since it's the top level image), we would fail and clearing or not clearing of the terminator does not matter. It simplifies the code for the next patch though. If we are nuking the rest of the backing chain (as we do in all cases today), we want to nuke the rest of the chain for the raw disk (since it would be invalid). After that we will always need to terminate the chain. So yes, change in the ordering is desired.
Reviewed-by: John Ferlan <jferlan@redhat.com>
(for the code regardless of your decision on placement)
Raw local files do not pass through the backing store detector and thus the code did not allocate the required backing store terminator for them. Previously the terminating element would be formatted into the XML since the default values used for the metadata allowed that. This is a regression since a693fdba0111ff which was not detected in the review. This patch also reverts all the changes in the test files. --- src/qemu/qemu_domain.c | 5 +++++ .../qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml | 1 + ...uhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 2 ++ .../qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 1 + ...muhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 1 + .../qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 1 + .../qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml | 1 + .../qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml | 1 + 11 files changed, 16 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0cdcb11c37..82671d99c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6167,6 +6167,11 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; } + /* terminate the chain for such images as the code below would do */ + if (!src->backingStore && + VIR_ALLOC(src->backingStore) < 0) + goto cleanup; + ret = 0; goto cleanup; } diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml index 0fa8d036b9..cd03d0e09b 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='vde' bus='virtio'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml index 135427fff5..7be75f977b 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='hda' bus='virtio'/> <readonly/> <shareable/> @@ -31,6 +32,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='hdb' bus='virtio'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml index e17c4e43b2..a83f1b5d73 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='hda' bus='virtio'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml index 326d312fa9..3e90207519 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='hda' bus='virtio'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml index 326d312fa9..3e90207519 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='hda' bus='virtio'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml index 16caeb3542..4c3ea3202b 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml @@ -33,6 +33,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='sdg' bus='scsi'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml index a6dbf0b1bd..493a615fd3 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='sdf' bus='scsi'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml index 6ccb88f140..3609819ea3 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='sdq' bus='usb'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml index b97c0b41e2..b88b220e33 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='vde' bus='virtio'/> <readonly/> <shareable/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml index 6422e1640d..c12d18f716 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml @@ -22,6 +22,7 @@ <disk type='file' device='disk'> <driver name='qemu' type='raw' cache='none'/> <source file='/dev/null'/> + <backingStore/> <target dev='sdf' bus='scsi'/> <readonly/> <shareable/> -- 2.14.3
On 11/24/2017 07:21 AM, Peter Krempa wrote:
Raw local files do not pass through the backing store detector and thus the code did not allocate the required backing store terminator for them. Previously the terminating element would be formatted into the XML since the default values used for the metadata allowed that. This is a regression since a693fdba0111ff which was not detected in the review.
This this is a bug fix to the bug that that patch was attempting to fix? I do see it being pointed out as a comment from review that there's a lot of backingStore removals... Perhaps better said - the initial patch was too aggressive but neglected to handle xxxx case? I'm sure there's a better way to wordsmith this particular attribution.
This patch also reverts all the changes in the test files. --- src/qemu/qemu_domain.c | 5 +++++
Comparing the files that changed in a693fdba0111ff...
.../qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml | 1 + ...uhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 2 ++ .../qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 1 + ...muhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 1 + .../qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 1 +
Why does qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml get the backingStore, but qemuhotplug-base-ccw-live-with-ccw-virtio.xml does not? I can understand why the "added" hotplug disks have it, but it's unclear why the "base" file doesn't for the non "with-2" test.
.../qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml | 1 + .../qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml | 1 + 11 files changed, 16 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0cdcb11c37..82671d99c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6167,6 +6167,11 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; }
+ /* terminate the chain for such images as the code below would do */ + if (!src->backingStore && + VIR_ALLOC(src->backingStore) < 0)
Should be able to fit on one line Reviewed-by: John Ferlan <jferlan@redhat.com> I'm OK w/ this and patch 2 in during freeze - the rules are a bit grey when it comes to fixing problems without bz's though...
On Wed, Nov 29, 2017 at 21:29:32 -0500, John Ferlan wrote:
On 11/24/2017 07:21 AM, Peter Krempa wrote:
Raw local files do not pass through the backing store detector and thus the code did not allocate the required backing store terminator for them. Previously the terminating element would be formatted into the XML since the default values used for the metadata allowed that. This is a regression since a693fdba0111ff which was not detected in the review.
This this is a bug fix to the bug that that patch was attempting to fix? I do see it being pointed out as a comment from review that there's a lot of backingStore removals... Perhaps better said - the initial patch was too aggressive but neglected to handle xxxx case? I'm sure there's a better way to wordsmith this particular attribution.
This patch also reverts all the changes in the test files. --- src/qemu/qemu_domain.c | 5 +++++
Comparing the files that changed in a693fdba0111ff...
.../qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml | 1 + ...uhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 2 ++ .../qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml | 1 + ...muhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 1 + .../qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml | 1 +
Why does qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml get the backingStore, but qemuhotplug-base-ccw-live-with-ccw-virtio.xml does not?
I can understand why the "added" hotplug disks have it, but it's unclear why the "base" file doesn't for the non "with-2" test.
This is because of the more complex test, where one device is attached to the domain "base-ccw-live-with-ccw-virtio" and then compared to "base-ccw-live-with-2-ccw-virtio" so that the first device can be detached. The device XMLs go through the hotplug code which adds the <backingStore> element, but the domain XMLs are not initialized via our qemuProcessPrepare.... code, thus the terminator was not added in that code path.
.../qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml | 1 + tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml | 1 + .../qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml | 1 + 11 files changed, 16 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0cdcb11c37..82671d99c6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6167,6 +6167,11 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; }
+ /* terminate the chain for such images as the code below would do */ + if (!src->backingStore && + VIR_ALLOC(src->backingStore) < 0)
Should be able to fit on one line
Reviewed-by: John Ferlan <jferlan@redhat.com>
I'm OK w/ this and patch 2 in during freeze - the rules are a bit grey when it comes to fixing problems without bz's though...
Well you may perceive it as 'grey' since there aren't any rules regarding this. This is upstream. Bugs exist even if they are not tracked by a bugzilla. In this case these patches spawned from a conversation on https://bugzilla.redhat.com/show_bug.cgi?id=1509110 I'll add it to the commit message
participants (2)
-
John Ferlan -
Peter Krempa