On Wed, Nov 20, 2019 at 11:44:52 +0100, Pavel Mores wrote: > Since the blockcommit operation is asynchronous, this has conceptually two > parts. First, we have to propagate the flag from qemuDomainBlockCommit() > (which was just ignoring it until now) to qemuBlockJobDiskNewCommit(). Then > it can be stored in the qemuBlockJobCommitData structure which holds > information necessary to finish the job asynchronously. > > Signed-off-by: Pavel Mores <pmores(a)redhat.com&gt; > --- > src/qemu/qemu_blockjob.c | 4 +++- > src/qemu/qemu_blockjob.h | 4 +++- > src/qemu/qemu_driver.c | 5 +++-- > 3 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c > index 5c294f8024..7d94a6ce38 100644 > --- a/src/qemu/qemu_blockjob.c > +++ b/src/qemu/qemu_blockjob.c > @@ -250,7 +250,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, > virDomainDiskDefPtr disk, > virStorageSourcePtr topparent, > virStorageSourcePtr top, > - virStorageSourcePtr base) > + virStorageSourcePtr base, > + bool delete_imgs) > { > qemuDomainObjPrivatePtr priv = vm->privateData; > g_autoptr(qemuBlockJobData) job = NULL; > @@ -273,6 +274,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, > job->data.commit.topparent = topparent; > job->data.commit.top = top; > job->data.commit.base = base; > + job->data.commit.deleteCommittedImages = delete_imgs; > > if (qemuBlockJobRegister(job, vm, disk, true) < 0) > return NULL; > diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h > index d8da918f2f..d77f1dcdb3 100644 > --- a/src/qemu/qemu_blockjob.h > +++ b/src/qemu/qemu_blockjob.h > @@ -85,6 +85,7 @@ struct _qemuBlockJobCommitData { > virStorageSourcePtr topparent; > virStorageSourcePtr top; > virStorageSourcePtr base; > + bool deleteCommittedImages; > }; > > > @@ -165,7 +166,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, > virDomainDiskDefPtr disk, > virStorageSourcePtr topparent, > virStorageSourcePtr top, > - virStorageSourcePtr base); > + virStorageSourcePtr base, > + bool delete_imgs); > > qemuBlockJobDataPtr > qemuBlockJobNewCreate(virDomainObjPtr vm, > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index dc14ec86a3..75458f5c8a 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -18496,10 +18496,10 @@ qemuDomainBlockCommit(virDomainPtr dom, > bool persistjob = false; > bool blockdev = false; > > - /* XXX Add support for COMMIT_DELETE */ > virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | > VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | > VIR_DOMAIN_BLOCK_COMMIT_RELATIVE | > + VIR_DOMAIN_BLOCK_COMMIT_DELETE | > VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1); > > if (!(vm = qemuDomainObjFromDomain(dom))) > @@ -18638,7 +18638,8 @@ qemuDomainBlockCommit(virDomainPtr dom, > goto endjob; > > if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, > - baseSource))) > + baseSource, > + flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE))) > goto endjob; I'd prefer if these last two hunks which enable the feature are in a separate commit at the end of the series, so that they enable it only once all the plumbing is in place.

On Wed, Nov 20, 2019 at 18:12:18 +0100, Pavel Mores wrote: > On Wed, Nov 20, 2019 at 12:14:11PM +0100, Peter Krempa wrote: > > On Wed, Nov 20, 2019 at 11:44:52 +0100, Pavel Mores wrote: > > > Since the blockcommit operation is asynchronous, this has conceptually two > > > parts. First, we have to propagate the flag from qemuDomainBlockCommit() > > > (which was just ignoring it until now) to qemuBlockJobDiskNewCommit(). Then > > > it can be stored in the qemuBlockJobCommitData structure which holds > > > information necessary to finish the job asynchronously. > > > > > > Signed-off-by: Pavel Mores <pmores(a)redhat.com&gt; > > > --- > > > src/qemu/qemu_blockjob.c | 4 +++- > > > src/qemu/qemu_blockjob.h | 4 +++- > > > src/qemu/qemu_driver.c | 5 +++-- > > > 3 files changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c > > > index 5c294f8024..7d94a6ce38 100644 > > > --- a/src/qemu/qemu_blockjob.c > > > +++ b/src/qemu/qemu_blockjob.c > > > @@ -250,7 +250,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, > > > virDomainDiskDefPtr disk, > > > virStorageSourcePtr topparent, > > > virStorageSourcePtr top, > > > - virStorageSourcePtr base) > > > + virStorageSourcePtr base, > > > + bool delete_imgs) > > > { > > > qemuDomainObjPrivatePtr priv = vm->privateData; > > > g_autoptr(qemuBlockJobData) job = NULL; > > > @@ -273,6 +274,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, > > > job->data.commit.topparent = topparent; > > > job->data.commit.top = top; > > > job->data.commit.base = base; > > > + job->data.commit.deleteCommittedImages = delete_imgs; > > > > > > if (qemuBlockJobRegister(job, vm, disk, true) < 0) > > > return NULL; > > > diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h > > > index d8da918f2f..d77f1dcdb3 100644 > > > --- a/src/qemu/qemu_blockjob.h > > > +++ b/src/qemu/qemu_blockjob.h > > > @@ -85,6 +85,7 @@ struct _qemuBlockJobCommitData { > > > virStorageSourcePtr topparent; > > > virStorageSourcePtr top; > > > virStorageSourcePtr base; > > > + bool deleteCommittedImages; > > > }; > > > > > > > > > @@ -165,7 +166,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, > > > virDomainDiskDefPtr disk, > > > virStorageSourcePtr topparent, > > > virStorageSourcePtr top, > > > - virStorageSourcePtr base); > > > + virStorageSourcePtr base, > > > + bool delete_imgs); > > > > > > qemuBlockJobDataPtr > > > qemuBlockJobNewCreate(virDomainObjPtr vm, > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > > index dc14ec86a3..75458f5c8a 100644 > > > --- a/src/qemu/qemu_driver.c > > > +++ b/src/qemu/qemu_driver.c > > > @@ -18496,10 +18496,10 @@ qemuDomainBlockCommit(virDomainPtr dom, > > > bool persistjob = false; > > > bool blockdev = false; > > > > > > - /* XXX Add support for COMMIT_DELETE */ > > > virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | > > > VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | > > > VIR_DOMAIN_BLOCK_COMMIT_RELATIVE | > > > + VIR_DOMAIN_BLOCK_COMMIT_DELETE | > > > VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1); > > > > > > if (!(vm = qemuDomainObjFromDomain(dom))) > > > @@ -18638,7 +18638,8 @@ qemuDomainBlockCommit(virDomainPtr dom, > > > goto endjob; > > > > > > if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, > > > - baseSource))) > > > + baseSource, > > > + flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE))) > > > goto endjob; > > > > I'd prefer if these last two hunks which enable the feature are in a > > separate commit at the end of the series, so that they enable it only > > once all the plumbing is in place. > > I tried to figure out how to do this but I'm afraid I don't see a very good > way. Here's my thinking: since code in patch 2 reads and uses > qemuBlockJobCommitData.deleteCommittedImages, patch 1 should make sure it's > there and it's initialised. So patch 1 should add deleteCommittedImages to > qemuBlockJobCommitData and qemuBlockJobDiskNewCommit() should set it to a > value. To get that value, it needs to take the extra argument. However, if > it does take the extra argument, qemuDomainBlockCommit() has to pass it. Just pass 'false' as the last argument of qemuBlockJobDiskNewCommit which will equal to the same behaviour as it did until now. Then pass the correct value along with enabling it in virCheckFlags after the code which actually uses the value is in place. > > So (without resorting to an initialisation to a dummy value), I can't really > see a natural way how to split this patch. Also, the actual enabling of the > feature doesn't happen anyway until patch 2 where deleteCommittedImages is > read and acted upon - until then, patch 1 - even as it is now - is just Well, the problem is that after the first patch the virCheckFlags macro will no longer reject VIR_DOMAIN_BLOCK_COMMIT_DELETE despite the code doing nothing (as in nothing besides storing the value) with it until later patches.

> setting a value that no-one reads. I'm not sure if it's worth the trouble - > am I overlooking something? > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

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))

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

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

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

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. I can see qemuDomainGetImageIds() in qemu_domain.c which seems somewhat close

to what you describe. It tries to get uid/gid from the virStorageSourcePtr itself, then its parent virStorageSourcePtr, then virDomainObjPtr and finally virQEMUDriverConfigPtr.

It's static in its file though, and I don't immediately see an easy way to pass it the virQEMUDriverConfigPtr. However, if it's OK to make it visible from

blockdev code, I could still call it along the lines of

qemuDomainGetImageIds(NULL, vm, p, NULL, &uid, &gid); as I can apparently get a virDomainObjPtr into the helper quite easily. > > + 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. > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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()?