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

[Peter Krempa]

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

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191121/92f25f0b/attachment-0001.sig>