Re: [libvirt] [PATCH 3/3] qemu: domain: Fix backing store terminator for non-backing local files
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(a)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...