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@xxxxxxxxxx>
> ---
> 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
>
