3:53 a.m.
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
3:53 a.m.
New subject: [PATCH 1/3] virdomainmomentobjlist.h: Convert to modern header style
Format the function prototypes the same way as in the .c file.
Signed-off-by: Peter Krempa <pkrempa(a)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
3:53 a.m.
New subject: [PATCH 2/3] conf: moment: Export helpers to create the virDomainMoment wrapper
Export 'virDomainMomentObjNew' and 'virDomainMomentObjFree' and define
the latter as autoptr cleanup function for 'virDomainMomentObj'.
Signed-off-by: Peter Krempa <pkrempa(a)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
3:53 a.m.
New subject: [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 <pkrempa(a)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
8:53 a.m.
New subject: [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 <pkrempa(a)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
9:03 a.m.
New subject: [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 <pkrempa(a)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 ;)
8:53 a.m.
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(a)redhat.com>
Michal
1046
days inactive
1047
days old
6 comments
2 participants
participants (2)
-
Michal Prívozník -
Peter Krempa