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&gt; 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...