[PATCH 0/2] qemu: Fix update of persistent confing on blockcommit
When commiting a image which is configured via a 'volume' storage source we fail to update the persistent definition. Peter Krempa (2): qemuBlockJobRewriteConfigDiskSource: Add debug statements when skipping disk update virStorageSourceIsSameLocation: Special-case storage sources of type 'volume' src/conf/storage_source_conf.c | 7 +++++++ src/qemu/qemu_blockjob.c | 15 ++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) -- 2.31.1
It makes it easier to see what's going on when trying to figure out why the disk definition was not updated on a finalized blockjob. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index faf9a9fb7d..6911a2a68d 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -667,14 +667,23 @@ qemuBlockJobRewriteConfigDiskSource(virDomainObj *vm, g_autoptr(virStorageSource) copy = NULL; virStorageSource *n; - if (!vm->newDef) + if (!vm->newDef) { + VIR_DEBUG("not updating disk '%s' in persistent definition: no persistent definition", + disk->dst); return; + } - if (!(persistDisk = virDomainDiskByTarget(vm->newDef, disk->dst))) + if (!(persistDisk = virDomainDiskByTarget(vm->newDef, disk->dst))) { + VIR_DEBUG("not updating disk '%s' in persistent definition: disk not present", + disk->dst); return; + } - if (!virStorageSourceIsSameLocation(disk->src, persistDisk->src)) + if (!virStorageSourceIsSameLocation(disk->src, persistDisk->src)) { + VIR_DEBUG("not updating disk '%s' in persistent definition: disk source doesn't match", + disk->dst); return; + } if (!(copy = virStorageSourceCopy(newsrc, true)) || virStorageSourceInitChainElement(copy, persistDisk->src, true) < 0) { -- 2.31.1
On Fri, Nov 26, 2021 at 02:07:53PM +0100, Peter Krempa wrote:
It makes it easier to see what's going on when trying to figure out why the disk definition was not updated on a finalized blockjob.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- src/qemu/qemu_blockjob.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index faf9a9fb7d..6911a2a68d 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -667,14 +667,23 @@ qemuBlockJobRewriteConfigDiskSource(virDomainObj *vm, g_autoptr(virStorageSource) copy = NULL; virStorageSource *n;
- if (!vm->newDef) + if (!vm->newDef) { + VIR_DEBUG("not updating disk '%s' in persistent definition: no persistent definition", + disk->dst); return; + }
- if (!(persistDisk = virDomainDiskByTarget(vm->newDef, disk->dst))) + if (!(persistDisk = virDomainDiskByTarget(vm->newDef, disk->dst))) { + VIR_DEBUG("not updating disk '%s' in persistent definition: disk not present", + disk->dst); return; + }
- if (!virStorageSourceIsSameLocation(disk->src, persistDisk->src)) + if (!virStorageSourceIsSameLocation(disk->src, persistDisk->src)) { + VIR_DEBUG("not updating disk '%s' in persistent definition: disk source doesn't match", + disk->dst); return; + }
if (!(copy = virStorageSourceCopy(newsrc, true)) || virStorageSourceInitChainElement(copy, persistDisk->src, true) < 0) { -- 2.31.1
The function is used also to compare virStorageSource which may not be resolved to the image at that point in which case the 'path' is not yet populated and the actual type is not yet set. This means that the function fails to consider two identical volume-based disks as pointing to the same thing. Add a special case for both images being type=volume in which case we compare only the pool/volume names. Closes: https://gitlab.com/libvirt/libvirt/-/issues/240 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/storage_source_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 44944e1dbd..c0acee189a 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -920,6 +920,13 @@ virStorageSourceIsSameLocation(virStorageSource *a, virStorageSourceIsEmpty(b)) return true; + /* for disk type=volume we must check just pool/volume names as they might + * not yet be resolved if e.g. we are comparing against the persistent def */ + if (a->type == VIR_STORAGE_TYPE_VOLUME && b->type == VIR_STORAGE_TYPE_VOLUME) { + return STREQ(a->srcpool->pool, b->srcpool->pool) && + STREQ(a->srcpool->volume, b->srcpool->volume); + } + if (virStorageSourceGetActualType(a) != virStorageSourceGetActualType(b)) return false; -- 2.31.1
On Fri, Nov 26, 2021 at 02:07:54PM +0100, Peter Krempa wrote:
The function is used also to compare virStorageSource which may not be resolved to the image at that point in which case the 'path' is not yet populated and the actual type is not yet set. This means that the function fails to consider two identical volume-based disks as pointing to the same thing.
Add a special case for both images being type=volume in which case we compare only the pool/volume names.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/240 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/storage_source_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 44944e1dbd..c0acee189a 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -920,6 +920,13 @@ virStorageSourceIsSameLocation(virStorageSource *a, virStorageSourceIsEmpty(b)) return true;
+ /* for disk type=volume we must check just pool/volume names as they might + * not yet be resolved if e.g. we are comparing against the persistent def */ + if (a->type == VIR_STORAGE_TYPE_VOLUME && b->type == VIR_STORAGE_TYPE_VOLUME) { + return STREQ(a->srcpool->pool, b->srcpool->pool) && + STREQ(a->srcpool->volume, b->srcpool->volume); + } +
Does this hold true even for disk sources that differ in the `mode` attribute? I'm just asking for clarity. If that was taken into consideration and the difference does not matter here, then this is Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
if (virStorageSourceGetActualType(a) != virStorageSourceGetActualType(b)) return false;
-- 2.31.1
On Fri, Nov 26, 2021 at 14:50:37 +0100, Martin Kletzander wrote:
On Fri, Nov 26, 2021 at 02:07:54PM +0100, Peter Krempa wrote:
The function is used also to compare virStorageSource which may not be resolved to the image at that point in which case the 'path' is not yet populated and the actual type is not yet set. This means that the function fails to consider two identical volume-based disks as pointing to the same thing.
Add a special case for both images being type=volume in which case we compare only the pool/volume names.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/240 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/storage_source_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 44944e1dbd..c0acee189a 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -920,6 +920,13 @@ virStorageSourceIsSameLocation(virStorageSource *a, virStorageSourceIsEmpty(b)) return true;
+ /* for disk type=volume we must check just pool/volume names as they might + * not yet be resolved if e.g. we are comparing against the persistent def */ + if (a->type == VIR_STORAGE_TYPE_VOLUME && b->type == VIR_STORAGE_TYPE_VOLUME) { + return STREQ(a->srcpool->pool, b->srcpool->pool) && + STREQ(a->srcpool->volume, b->srcpool->volume); + } +
Does this hold true even for disk sources that differ in the `mode` attribute? I'm just asking for clarity. If that was taken into consideration and the difference does not matter here, then this is
The idea is that the function should return true if both are pointing to the same backing, in those terms the 'mode' is irrelevant as it's still the same image. This code is used in blockjob finalization, where it doesn't matter at all and in cdrom image changing code, where it very theoretically could be used to change the mode, but it's questionable whether that would even make sense in any real usage.
On Mon, Nov 29, 2021 at 02:31:55PM +0100, Peter Krempa wrote:
On Fri, Nov 26, 2021 at 14:50:37 +0100, Martin Kletzander wrote:
On Fri, Nov 26, 2021 at 02:07:54PM +0100, Peter Krempa wrote:
The function is used also to compare virStorageSource which may not be resolved to the image at that point in which case the 'path' is not yet populated and the actual type is not yet set. This means that the function fails to consider two identical volume-based disks as pointing to the same thing.
Add a special case for both images being type=volume in which case we compare only the pool/volume names.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/240 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/storage_source_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 44944e1dbd..c0acee189a 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -920,6 +920,13 @@ virStorageSourceIsSameLocation(virStorageSource *a, virStorageSourceIsEmpty(b)) return true;
+ /* for disk type=volume we must check just pool/volume names as they might + * not yet be resolved if e.g. we are comparing against the persistent def */ + if (a->type == VIR_STORAGE_TYPE_VOLUME && b->type == VIR_STORAGE_TYPE_VOLUME) { + return STREQ(a->srcpool->pool, b->srcpool->pool) && + STREQ(a->srcpool->volume, b->srcpool->volume); + } +
Does this hold true even for disk sources that differ in the `mode` attribute? I'm just asking for clarity. If that was taken into consideration and the difference does not matter here, then this is
The idea is that the function should return true if both are pointing to the same backing, in those terms the 'mode' is irrelevant as it's still the same image.
This code is used in blockjob finalization, where it doesn't matter at all and in cdrom image changing code, where it very theoretically could be used to change the mode, but it's questionable whether that would even make sense in any real usage.
OK, I had to ask because I read the documentation around 10 times and wasn't sure whether it can change where the source points to, but it looks like it does no, just changes how to represent the path to the device. Sorry for the noise.
participants (2)
-
Martin Kletzander -
Peter Krempa