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

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(a)redhat.com&gt; > > --- > > 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()?