On Thu, Nov 21, 2019 at 08:38:13AM +0100, Peter Krempa wrote:
> 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@xxxxxxxxxx>
> > > > ---
> > > > 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.
Ah okay then, makes sense.
> > 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@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
