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@xxxxxxxxxx> > > 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
