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

[Pavel Mores]

On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
> 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.

One more thing comes to my mind though - is VIR_WARN() enough as far as
reporting the error goes?  Would it make sense to replace it with e.g.
virReportError()?