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