Re: [libvirt RFC 12/24] qemu_snapshot: rework snapshot children deletion
On Tue, Aug 23, 2022 at 18:32:15 +0200, Pavel Hrdina wrote:
This simplifies the code a bit by reusing existing parts that
deletes
a single snapshot.
You should mention that the tradeoff is that some steps will get
executed multiple times.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 69 ++++++++++++++++++++++------------------
1 file changed, 38 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index d40d75e75d..db04244018 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2310,6 +2310,33 @@ qemuSnapshotDeleteSingle(virDomainObj *vm,
}
+struct qemuSnapshotDeleteAllData {
+ virDomainObj *vm;
+ virQEMUDriver *driver;
+ bool metadata_only;
+ int error;
+};
+
+
+static int
+qemuSnapshotDeleteAll(void *payload,
+ const char *name G_GNUC_UNUSED,
+ void *opaque)
+{
+ int error;
+ virDomainMomentObj *snap = payload;
+ struct qemuSnapshotDeleteAllData *data = opaque;
+
+ error = qemuSnapshotDeleteSingle(data->vm, snap, data->driver,
+ data->metadata_only);
+
+ if (error != 0 && data->error != 0)
So 'data->error' is initialized to '0' in the caller. By the logic
above
you'll never write to it if 'error' will actually carry a non-zero
value.
Since it's used as a glorified boolean (having only '0' or '-1'
stored
in it) I think you can drop the '&& data->error != 0) altogether and
always update it when an error occured.
+ data->error = error;
+
+ return 0;
+}
+
+
static int
qemuSnapshotDeleteChildren(virDomainObj *vm,
virDomainMomentObj *snap,
@@ -2317,42 +2344,22 @@ qemuSnapshotDeleteChildren(virDomainObj *vm,
bool metadata_only,
unsigned int flags)
{
- virQEMUMomentRemove rem;
- g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+ struct qemuSnapshotDeleteAllData data = { 0 };
[1]. Also I'd go with c99 style init ...
- rem.driver = driver;
- rem.vm = vm;
- rem.metadata_only = metadata_only;
- rem.err = 0;
- rem.current = virDomainSnapshotGetCurrent(vm->snapshots);
- rem.found = false;
- rem.momentDiscard = qemuDomainSnapshotDiscard;
- virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll,
- &rem);
- if (rem.err < 0)
+ data.vm = vm;
+ data.driver = driver;
+ data.metadata_only = metadata_only;
... to make this more obvious.
+
+ virDomainMomentForEachDescendant(snap, qemuSnapshotDeleteAll, &data);
+
+ if (data.error < 0)
return -1;
- if (rem.found) {
- qemuSnapshotSetCurrent(vm, snap);
- if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
- if (qemuDomainSnapshotWriteMetadata(vm, snap,
- driver->xmlopt,
- cfg->snapshotDir) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to set snapshot '%s' as
current"),
- snap->def->name);
- virDomainSnapshotSetCurrent(vm->snapshots, NULL);
- return -1;
- }
- }
+ if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) {
+ return qemuSnapshotDeleteSingle(vm, snap, driver, metadata_only);
Preferrably check that qemuSnapshotDeleteSingle returns -1 and return
failure here instead ...
}
- if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
- virDomainMomentDropChildren(snap);
- return 0;
- }
-
- return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
+ return 0;
So that we have the usual control flow.
}
--
2.37.2