[libvirt] [PATCH v2 1/4] qemu: block: propagate the delete flag to where it can actually be used

[Pavel Mores]

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 at redhat.com>
> > ---
> >  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.

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
setting a value that no-one reads.  I'm not sure if it's worth the trouble -
am I overlooking something?