[libvirt PATCH 0/2] don't require disk-only flag when creating external snapshots
Pavel Hrdina (2): qemu_snapshot: create: refactor external snapshot detection qemu_snapshot: create: don't require disk-only flag for offline external snapshot src/qemu/qemu_snapshot.c | 44 ++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) -- 2.43.0
Introduce new function qemuSnapshotCreateUseExternal() that will return true if we will use external snapshots as default location. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 73ff533827..ab7b47319b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1576,6 +1576,20 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm, } +static bool +qemuSnapshotCreateUseExternal(virDomainSnapshotDef *def, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) + return true; + + if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + return true; + + return false; +} + + static int qemuSnapshotCreateAlignDisks(virDomainObj *vm, virDomainSnapshotDef *def, @@ -1584,7 +1598,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, { g_autofree char *xml = NULL; qemuDomainObjPrivate *priv = vm->privateData; - virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT; /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ @@ -1604,17 +1618,19 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, return -1; } - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + if (qemuSnapshotCreateUseExternal(def, flags)) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - if (virDomainObjIsActive(vm)) - def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT; - else - def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF; - def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; - } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); - align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + if (virDomainObjIsActive(vm)) + def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT; + else + def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF; + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; + } } else { + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; def->state = virDomainObjGetState(vm, NULL); if (virDomainObjIsActive(vm) && -- 2.43.0
On Tue, Jan 30, 2024 at 12:33:50 +0100, Pavel Hrdina wrote:
Introduce new function qemuSnapshotCreateUseExternal() that will return true if we will use external snapshots as default location.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 73ff533827..ab7b47319b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1576,6 +1576,20 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm, }
Please document the desired logic behind the return value.
+static bool +qemuSnapshotCreateUseExternal(virDomainSnapshotDef *def, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) + return true; + + if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + return true; + + return false; +} + + static int qemuSnapshotCreateAlignDisks(virDomainObj *vm, virDomainSnapshotDef *def,
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Historically creating offline external snapshot required disk-only flag as well. Now when user requests new snapshot for offline VM and at least one disk is specified to use external snapshot we will no longer require disk-only flag as all other not specified disk will use external snapshots as well. Resolves: https://issues.redhat.com/browse/RHEL-22797 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index ab7b47319b..d089f70d4e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1577,15 +1577,25 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm, static bool -qemuSnapshotCreateUseExternal(virDomainSnapshotDef *def, +qemuSnapshotCreateUseExternal(virDomainObj *vm, + virDomainSnapshotDef *def, unsigned int flags) { + ssize_t i; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) return true; if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) return true; + if (!virDomainObjIsActive(vm)) { + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + return true; + } + } + return false; } @@ -1618,7 +1628,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, return -1; } - if (qemuSnapshotCreateUseExternal(def, flags)) { + if (qemuSnapshotCreateUseExternal(vm, def, flags)) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; def->state = virDomainObjGetState(vm, NULL); -- 2.43.0
On Tue, Jan 30, 2024 at 12:33:51 +0100, Pavel Hrdina wrote:
Historically creating offline external snapshot required disk-only flag as well. Now when user requests new snapshot for offline VM and at least one disk is specified to use external snapshot we will no longer require disk-only flag as all other not specified disk will use external snapshots as well.
Resolves: https://issues.redhat.com/browse/RHEL-22797 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index ab7b47319b..d089f70d4e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1577,15 +1577,25 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm,
static bool -qemuSnapshotCreateUseExternal(virDomainSnapshotDef *def, +qemuSnapshotCreateUseExternal(virDomainObj *vm, + virDomainSnapshotDef *def, unsigned int flags) { + ssize_t i;
size_t, def->ndisks is also size_t
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) return true;
if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) return true;
+ if (!virDomainObjIsActive(vm)) { + for (i = 0; i < def->ndisks; i++) {
I'd mention that $OTHER_FUNCTION guarantees that we don't have a combination of _LOCATION_INTERNAL and _LOCATION_EXTERNAL here. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa