6:21 a.m.
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
6:21 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain
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
8:25 p.m.
New subject: [libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain
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(a)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
6:51 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: domain: Refactor control flow in qemuDomainDetermineDiskChain
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(a)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
>
6:21 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: process: Move handling of non-backing files into qemuDomainDetermineDiskChain
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
8:25 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: process: Move handling of non-backing files into qemuDomainDetermineDiskChain
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(a)redhat.com>
(for the code regardless of your decision on placement)
6:44 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: process: Move handling of non-backing files into qemuDomainDetermineDiskChain
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(a)redhat.com>
(for the code regardless of your decision on placement)
6:21 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: domain: Fix backing store terminator for non-backing local files
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
8:29 p.m.
New subject: [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...
8:57 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: domain: Fix backing store terminator for non-backing local files
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(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...
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
2579
days inactive
2585
days old
9 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa