[libvirt] [PATCH v3 2/4] qemu: block: use the delete flag to delete snapshot images if requested

[Peter Krempa]

On Thu, Nov 21, 2019 at 11:00:45 +0100, Pavel Mores wrote:
> When blockcommit finishes successfully, one of the
> qemuBlockJobProcessEventCompletedCommit() and
> qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
> This is where the delete flag (stored in qemuBlockJobCommitData since the
> previous commit) can actually be used to delete the committed snapshot
> images if requested.
> 
> We use virFileRemove() instead of a simple unlink() to cover the case where
> the image to be removed is on an NFS volume.  To acquire the uid/gid arguments
> for the virFileRemove() call, we call qemuDomainGetImageIds() which was
> previously static in its file so we first have to export it.
> 
> Signed-off-by: Pavel Mores <pmores at redhat.com>
> ---
>  src/qemu/qemu_blockjob.c | 39 +++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.c   |  2 +-
>  src/qemu/qemu_domain.h   |  6 ++++++
>  3 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 7d94a6ce38..1bf23dac3c 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -965,6 +965,37 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
>  }
>  
>  
> +/**
> + * Helper for qemuBlockJobProcessEventCompletedCommit() and
> + * qemuBlockJobProcessEventCompletedActiveCommit().  Relies on adjustments
> + * these functions perform on the 'backingStore' chain to function correctly.

Preferably use the format of the comments we use elsewhere too.

> + *
> + * TODO look into removing backing store for non-local snapshots too
> + */
> +static void
> +qemuBlockJobUnlinkCommittedStorage(virQEMUDriverPtr driver,

You could name it more generic as this same operation may be added for
other block jobs in the future.

E.g. qemuBlockJobDeleteImages.

> +                                   virDomainObjPtr vm,
> +                                   virDomainDiskDefPtr disk,
> +                                   virStorageSourcePtr top)
> +{
> +    virStorageSourcePtr p = top;
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    uid_t uid;
> +    gid_t gid;
> +
> +    for (; p != NULL; p = p->backingStore) {
> +        if (virStorageSourceIsLocalStorage(p)) {

This also returns true for 'VIR_STORAGE_TYPE_FILE' but we certainly
don't want to unlink those.

> +
> +            qemuDomainGetImageIds(cfg, vm, p, disk->src, &uid, &gid);
> +
> +            if (virFileRemove(p->path, uid, gid) < 0) {
> +                VIR_WARN("Unable to remove snapshot image file %s (%s)",

Add apostrophes around the first %s -> '%s'

> +                         p->path, g_strerror(errno));
> +            }
> +        }
> +    }
> +}
> +
>  /**
>   * qemuBlockJobProcessEventCompletedCommit:
>   * @driver: qemu driver object
> @@ -1031,6 +1062,10 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver,
>      job->data.commit.topparent->backingStore = job->data.commit.base;
>  
>      qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->data.commit.top);
> +
> +    if (job->data.commit.deleteCommittedImages)
> +        qemuBlockJobUnlinkCommittedStorage(driver, vm, job->disk, job->data.commit.top);
> +
>      virObjectUnref(job->data.commit.top);
>      job->data.commit.top = NULL;
>  
> @@ -1121,6 +1156,10 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr driver,
>      job->disk->src->readonly = job->data.commit.top->readonly;
>  
>      qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->data.commit.top);
> +
> +    if (job->data.commit.deleteCommittedImages)
> +        qemuBlockJobUnlinkCommittedStorage(driver, vm, job->disk, job->data.commit.top);
> +
>      virObjectUnref(job->data.commit.top);
>      job->data.commit.top = NULL;
>      /* the mirror element does not serve functional purpose for the commit job */
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 262b74d1ab..07bf8f5a54 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10133,7 +10133,7 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver,
>      priv->ncleanupCallbacks_max = 0;
>  }
>  
> -static void
> +void
>  qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg,
>                        virDomainObjPtr vm,
>                        virStorageSourcePtr src,

Preferably export this in a separate commit.

> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 98a9540275..f66610c7d3 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -834,6 +834,12 @@ bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>  const char *qemuDomainDiskNodeFormatLookup(virDomainObjPtr vm,
>                                             const char *disk);
>  
> +void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg,
> +                      virDomainObjPtr vm,
> +                      virStorageSourcePtr src,
> +                      virStorageSourcePtr parentSrc,
> +                      uid_t *uid, gid_t *gid);
> +
>  int qemuDomainStorageFileInit(virQEMUDriverPtr driver,
>                                virDomainObjPtr vm,
>                                virStorageSourcePtr src,
> -- 
> 2.21.0
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list