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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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... -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.