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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
