[libvirt RFC 12/24] qemu_snapshot: rework snapshot children deletion

[Peter Krempa]

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 at 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
>