[PATCH for 8.0 0/3] Fix creation of snapshots without libvirt metadata
After a recent unreleased refactor we don't remove the temporary snapshot object from the list when finishing a snapshot without metadata thus we should fix it before releasing a buggy release. This patchset fixes it by not adding it into the list in the first place. Peter Krempa (3): virdomainmomentobjlist.h: Convert to modern header style conf: moment: Export helpers to create the virDomainMoment wrapper qemuSnapshotCreate: Don't insert snapshot into list with VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA src/conf/virdomainmomentobjlist.c | 4 +- src/conf/virdomainmomentobjlist.h | 121 +++++++++++++++++++----------- src/libvirt_private.syms | 2 + src/qemu/qemu_snapshot.c | 25 +++--- 4 files changed, 97 insertions(+), 55 deletions(-) -- 2.31.1
Format the function prototypes the same way as in the .c file. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainmomentobjlist.h | 113 ++++++++++++++++++------------ 1 file changed, 70 insertions(+), 43 deletions(-) diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index dd8f798301..e42f9a7e9e 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -50,24 +50,33 @@ struct _virDomainMomentObj { virDomainMomentObj *first_child; /* NULL if no children */ }; -int virDomainMomentForEachChild(virDomainMomentObj *moment, - virHashIterator iter, - void *data); -int virDomainMomentForEachDescendant(virDomainMomentObj *moment, - virHashIterator iter, - void *data); -void virDomainMomentDropParent(virDomainMomentObj *moment); -void virDomainMomentDropChildren(virDomainMomentObj *moment); -void virDomainMomentMoveChildren(virDomainMomentObj *from, - virDomainMomentObj *to); -void virDomainMomentLinkParent(virDomainMomentObjList *moments, - virDomainMomentObj *moment); - -virDomainMomentObjList *virDomainMomentObjListNew(void); -void virDomainMomentObjListFree(virDomainMomentObjList *moments); - -virDomainMomentObj *virDomainMomentAssignDef(virDomainMomentObjList *moments, - virDomainMomentDef *def); +int +virDomainMomentForEachChild(virDomainMomentObj *moment, + virHashIterator iter, + void *data); +int +virDomainMomentForEachDescendant(virDomainMomentObj *moment, + virHashIterator iter, + void *data); +void +virDomainMomentDropParent(virDomainMomentObj *moment); +void +virDomainMomentDropChildren(virDomainMomentObj *moment); +void +virDomainMomentMoveChildren(virDomainMomentObj *from, + virDomainMomentObj *to); +void +virDomainMomentLinkParent(virDomainMomentObjList *moments, + virDomainMomentObj *moment); + +virDomainMomentObjList * +virDomainMomentObjListNew(void); +void +virDomainMomentObjListFree(virDomainMomentObjList *moments); + +virDomainMomentObj * +virDomainMomentAssignDef(virDomainMomentObjList *moments, + virDomainMomentDef *def); /* Various enum bits that map to public API filters. Note that the * values of the internal bits are not the same as the public ones for @@ -97,28 +106,46 @@ typedef enum { VIR_DOMAIN_MOMENT_FILTERS_METADATA | \ VIR_DOMAIN_MOMENT_FILTERS_LEAVES) -int virDomainMomentObjListGetNames(virDomainMomentObjList *moments, - virDomainMomentObj *from, - char **const names, - int maxnames, - unsigned int moment_flags, - virDomainMomentObjListFilter filter, - unsigned int filter_flags); -virDomainMomentObj *virDomainMomentFindByName(virDomainMomentObjList *moments, - const char *name); -int virDomainMomentObjListSize(virDomainMomentObjList *moments); -virDomainMomentObj *virDomainMomentGetCurrent(virDomainMomentObjList *moments); -const char *virDomainMomentGetCurrentName(virDomainMomentObjList *moments); -void virDomainMomentSetCurrent(virDomainMomentObjList *moments, - virDomainMomentObj *moment); -bool virDomainMomentObjListRemove(virDomainMomentObjList *moments, - virDomainMomentObj *moment); -void virDomainMomentObjListRemoveAll(virDomainMomentObjList *moments); -int virDomainMomentForEach(virDomainMomentObjList *moments, - virHashIterator iter, - void *data); -int virDomainMomentUpdateRelations(virDomainMomentObjList *moments); -int virDomainMomentCheckCycles(virDomainMomentObjList *list, - virDomainMomentDef *def, - const char *domname); -virDomainMomentObj *virDomainMomentFindLeaf(virDomainMomentObjList *list); +int +virDomainMomentObjListGetNames(virDomainMomentObjList *moments, + virDomainMomentObj *from, + char **const names, + int maxnames, + unsigned int moment_flags, + virDomainMomentObjListFilter filter, + unsigned int filter_flags); +virDomainMomentObj * +virDomainMomentFindByName(virDomainMomentObjList *moments, + const char *name); +int +virDomainMomentObjListSize(virDomainMomentObjList *moments); + +virDomainMomentObj * +virDomainMomentGetCurrent(virDomainMomentObjList *moments); +const char * +virDomainMomentGetCurrentName(virDomainMomentObjList *moments); +void +virDomainMomentSetCurrent(virDomainMomentObjList *moments, + virDomainMomentObj *moment); + +bool +virDomainMomentObjListRemove(virDomainMomentObjList *moments, + virDomainMomentObj *moment); +void +virDomainMomentObjListRemoveAll(virDomainMomentObjList *moments); + +int +virDomainMomentForEach(virDomainMomentObjList *moments, + virHashIterator iter, + void *data); + +int +virDomainMomentUpdateRelations(virDomainMomentObjList *moments); + +int +virDomainMomentCheckCycles(virDomainMomentObjList *list, + virDomainMomentDef *def, + const char *domname); + +virDomainMomentObj * +virDomainMomentFindLeaf(virDomainMomentObjList *list); -- 2.31.1
Export 'virDomainMomentObjNew' and 'virDomainMomentObjFree' and define the latter as autoptr cleanup function for 'virDomainMomentObj'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainmomentobjlist.c | 4 ++-- src/conf/virdomainmomentobjlist.h | 8 ++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index 60f7eec106..8993c2310b 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -205,7 +205,7 @@ virDomainMomentMoveChildren(virDomainMomentObj *from, } -static virDomainMomentObj * +virDomainMomentObj * virDomainMomentObjNew(void) { virDomainMomentObj *moment; @@ -218,7 +218,7 @@ virDomainMomentObjNew(void) } -static void +void virDomainMomentObjFree(virDomainMomentObj *moment) { if (!moment) diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index e42f9a7e9e..d2ab3b46b1 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -50,6 +50,14 @@ struct _virDomainMomentObj { virDomainMomentObj *first_child; /* NULL if no children */ }; +virDomainMomentObj * +virDomainMomentObjNew(void); + +void +virDomainMomentObjFree(virDomainMomentObj *moment); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainMomentObj, virDomainMomentObjFree); + int virDomainMomentForEachChild(virDomainMomentObj *moment, virHashIterator iter, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ee14b99d88..5b76e66e61 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1175,6 +1175,8 @@ virDomainMomentDropParent; virDomainMomentForEachChild; virDomainMomentForEachDescendant; virDomainMomentMoveChildren; +virDomainMomentObjFree; +virDomainMomentObjNew; # conf/virdomainobjlist.h -- 2.31.1
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 <pkrempa@redhat.com> --- 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, &flags) < 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 = &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
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 <pkrempa@redhat.com> --- 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
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 <pkrempa@redhat.com> --- 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 ;)
On 1/11/22 10:53, Peter Krempa wrote:
After a recent unreleased refactor we don't remove the temporary snapshot object from the list when finishing a snapshot without metadata thus we should fix it before releasing a buggy release.
This patchset fixes it by not adding it into the list in the first place.
Peter Krempa (3): virdomainmomentobjlist.h: Convert to modern header style conf: moment: Export helpers to create the virDomainMoment wrapper qemuSnapshotCreate: Don't insert snapshot into list with VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA
src/conf/virdomainmomentobjlist.c | 4 +- src/conf/virdomainmomentobjlist.h | 121 +++++++++++++++++++----------- src/libvirt_private.syms | 2 + src/qemu/qemu_snapshot.c | 25 +++--- 4 files changed, 97 insertions(+), 55 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa