Re: [PATCH 3/3] qemuSnapshotCreate: Don't insert snapshot into list with VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA
On Tue, Jan 11, 2022 at 15:53:22 +0100, Michal Prívozník wrote: > On 1/11/22 10:53, Peter Krempa wrote: [...] > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2039131 > > Signed-off-by: Peter Krempa > > --- > > src/qemu/qemu_snapshot.c | 25 +++-- > > 1 file changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c > > index 9a5d3e60aa..e9fc9051c1 100644 > > --- a/src/qemu/qemu_snapshot.c > > +++ b/src/qemu/qemu_snapshot.c > > @@ -1756,7 +1756,7 @@ qemuSnapshotCreate(virDomainObj *vm, > > virQEMUDriverConfig *cfg, > > unsigned int flags) > > { > > - > > +g_autoptr(virDomainMomentObj) noMetadataSnap = NULL; > > Nitpick, this variable is used as a bool later, which creates double > negative conditions like !noMetadataSnap. But I'm failing to suggest > anything better. I went with 'tmpsnap'. It is indeed temporary ;)
Re: [PATCH 3/3] qemuSnapshotCreate: Don't insert snapshot into list with VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA
On 1/11/22 10:53, Peter Krempa wrote: > Our approach to snapshots without metadata was to insert them to the > snapshot list and then later remove them from the list when the flag is > present. > > This quirky logic was broken in a recent refactor of the snapshot code > causing that the snapshot stayed inserted in the snapshot list. > > Recent refactor of the snapshot code didn't faithfully relocate this > logic to the new function. > > Rather than attempting to restore the quirky logic of adding and then > removing the object, don't add the snapshot into the list at all when > the user doesn't want metadata. > > We achieve this by creating a temporary 'virDomainMomentObj' wrapper > which is not inserted into the list and using that instead of calling > virDomainSnapshotAssignDef. > > Fixes: 9bad0fb809b > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2039131 > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_snapshot.c | 25 +++-- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c > index 9a5d3e60aa..e9fc9051c1 100644 > --- a/src/qemu/qemu_snapshot.c > +++ b/src/qemu/qemu_snapshot.c > @@ -1756,7 +1756,7 @@ qemuSnapshotCreate(virDomainObj *vm, > virQEMUDriverConfig *cfg, > unsigned int flags) > { > - > +g_autoptr(virDomainMomentObj) noMetadataSnap = NULL; Nitpick, this variable is used as a bool later, which creates double negative conditions like !noMetadataSnap. But I'm failing to suggest anything better. Michal
[PATCH 3/3] qemuSnapshotCreate: Don't insert snapshot into list with VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA
Our approach to snapshots without metadata was to insert them to the snapshot list and then later remove them from the list when the flag is present. This quirky logic was broken in a recent refactor of the snapshot code causing that the snapshot stayed inserted in the snapshot list. Recent refactor of the snapshot code didn't faithfully relocate this logic to the new function. Rather than attempting to restore the quirky logic of adding and then removing the object, don't add the snapshot into the list at all when the user doesn't want metadata. We achieve this by creating a temporary 'virDomainMomentObj' wrapper which is not inserted into the list and using that instead of calling virDomainSnapshotAssignDef. Fixes: 9bad0fb809b Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2039131 Signed-off-by: Peter Krempa --- src/qemu/qemu_snapshot.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9a5d3e60aa..e9fc9051c1 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1756,7 +1756,7 @@ qemuSnapshotCreate(virDomainObj *vm, virQEMUDriverConfig *cfg, unsigned int flags) { - +g_autoptr(virDomainMomentObj) noMetadataSnap = NULL; virDomainMomentObj *snap = NULL; virDomainMomentObj *current = NULL; virDomainSnapshotPtr ret = NULL; @@ -1767,16 +1767,20 @@ qemuSnapshotCreate(virDomainObj *vm, if (qemuSnapshotPrepare(vm, def, ) < 0) return NULL; -if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) -return NULL; - -virObjectRef(def); +if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA) { +snap = noMetadataSnap = virDomainMomentObjNew(); +snap->def = >parent; +} else { +if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) +return NULL; -current = virDomainSnapshotGetCurrent(vm->snapshots); -if (current) { -snap->def->parent_name = g_strdup(current->def->name); +if ((current = virDomainSnapshotGetCurrent(vm->snapshots))) { +snap->def->parent_name = g_strdup(current->def->name); +} } +virObjectRef(def); + /* actually do the snapshot */ if (virDomainObjIsActive(vm)) { if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY || @@ -1804,7 +1808,7 @@ qemuSnapshotCreate(virDomainObj *vm, } } -if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { +if (!noMetadataSnap) { qemuSnapshotSetCurrent(vm, snap); if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0) @@ -1818,7 +1822,8 @@ qemuSnapshotCreate(virDomainObj *vm, return ret; error: -virDomainSnapshotObjListRemove(vm->snapshots, snap); +if (!noMetadataSnap) +virDomainSnapshotObjListRemove(vm->snapshots, snap); return NULL; } -- 2.31.1