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

[Peter Krempa]

On Wed, Nov 20, 2019 at 11:44:53 +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.
> 
> Signed-off-by: Pavel Mores <pmores at redhat.com>
> ---
>  src/qemu/qemu_blockjob.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 7d94a6ce38..cf221a2839 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -965,6 +965,31 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
>  }
>  
>  
> +/**
> + * Helper for qemuBlockJobProcessEventCompletedCommit() and
> + * qemuBlockJobProcessEventCompletedActiveCommit().  Relies on adjustments
> + * these functions perform on the 'backingStore' chain to function correctly.
> + *
> + * TODO look into removing backing store for non-local snapshots too
> + */
> +static void
> +qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
> +{
> +    virStorageSourcePtr p = top;
> +    const size_t errmsg_len = 128;
> +    char errmsg_buf[errmsg_len];
> +    char *errmsg;
> +
> +    for (; p != NULL; p = p->backingStore) {
> +        if (virStorageSourceIsLocalStorage(p))

The above condition has a multiline body, so it must be enclosed in a
block.

> +            if (unlink(p->path) < 0) {

I was considering whether we should use virFileRemove which will also
work properly on root-squashed NFS. I'm not sure though how easy it will
be to figure out the correct uid and gid inside this helper.

If you are interrested into investigating if it's possible, there should
be some prior art in the qemu driver where we try to get the uid/gid frm
virStorageSource if it's configured, then fall back to the domain
uid/gid of the process.

> +                errmsg = strerror_r(errno, errmsg_buf, errmsg_len);

Please use g_strerror(); It does not require any of the buffers and
stuff.

> +                VIR_WARN("Unable to remove snapshot image file %s (%s)",
> +                         p->path, errmsg);
> +            }
> +    }

The rest looks good.