On Tue, Dec 13, 2022 at 18:12:38 +0100, Pavel Hrdina wrote:
> On Tue, Dec 13, 2022 at 10:11:36AM +0100, Peter Krempa wrote:
> > On Tue, Dec 13, 2022 at 10:06:41 +0100, Peter Krempa wrote:
> > > On Thu, Dec 08, 2022 at 14:30:46 +0100, Pavel Hrdina wrote:
> > > > The created job will be needed by external snapshot delete code so
> > > > rework qemuBlockCommit to return that pointer.
> > > >
> > > > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > > > ---
> > > > src/qemu/qemu_block.c | 42 +++++++++++++++++++++++-------------------
> > > > src/qemu/qemu_block.h | 2 +-
> > > > src/qemu/qemu_driver.c | 5 ++++-
> > > > 3 files changed, 28 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> > > > index 24c82377b0..0a0d942e71 100644
> > > > --- a/src/qemu/qemu_block.c
> > > > +++ b/src/qemu/qemu_block.c
> > > > @@ -3201,8 +3201,12 @@ qemuBlockExportAddNBD(virDomainObj *vm,
> > > > /* The autofinalize argument controls if the qemu block job will be automatically
> > > > * finalized. This is used when deleting external snapshots where we need to
> > > > * disable automatic finalization for some use-case. The default value passed
> > > > - * to this argument should be VIR_TRISTATE_BOOL_YES.*/
> > > > -int
> > > > + * to this argument should be VIR_TRISTATE_BOOL_YES.
> > > > + *
> > > > + * Returns qemuBlockJobData pointer on success, NULL on error. Caller is responsible
> > > > + * to call virObjectUnref on the pointer.
> > > > + */
> > >
> > > See, you are adding and adding to this but you didn't start off properly
> > > ;)
> > >
> > > > +qemuBlockJobData *
> > > > qemuBlockCommit(virDomainObj *vm,
> > > > virDomainDiskDef *disk,
> > > > virStorageSource *baseSource,
> > >
> > > [...]
> > >
> > > > @@ -3367,7 +3371,7 @@ qemuBlockCommit(virDomainObj *vm,
> > > > }
> > > > qemuBlockJobStarted(job, vm);
> > > >
> > > > - return 0;
> > > > + return job;
> > >
> > > [...]
> > >
> > > >
> > > > error:
> > > > if (clean_access) {
> > >
> > > [...]
> > >
> > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > > index 12fca22616..53533062e1 100644
> > > > --- a/src/qemu/qemu_driver.c
> > > > +++ b/src/qemu/qemu_driver.c
> > > > @@ -14999,6 +14999,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
> > > > virStorageSource *topSource;
> > > > virStorageSource *baseSource = NULL;
> > > > virStorageSource *top_parent = NULL;
> > > > + g_autoptr(qemuBlockJobData) job = NULL;
> > >
> > > [2]
> > >
> > > >
> > > > virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
> > > > VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
> > > > @@ -15030,9 +15031,11 @@ qemuDomainBlockCommit(virDomainPtr dom,
> > > > base, disk->dst, NULL)))
> > > > goto endjob;
> > > >
> > > > - ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent,
> > > > + job = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent,
> > > > bandwidth, VIR_ASYNC_JOB_NONE, VIR_TRISTATE_BOOL_YES,
> > > > flags);
> > > > + if (job)
> > > > + ret = 0;
> > > >
> > > > endjob:
> > >
> > >
> > > Okay so this somehow isn't adding up for me. Prior to this patch before
> > > you returned the job [1] there wasn't anything to unref it [2]. So how
> > > come it's now needed if you don't increase reference count?
> > >
> > > Looking back at the refactoring patches moving the code out I think I
> > > see the problem there.
> > >
> > > Either way qemuBlockJobStartupFinalize() is the proper function to call
> > > rather than automatically unref it.
> >
> > Once you fix patch 2/30 to call qemuBlockJobStartupFinalize in both
> > cases, this patch will also need to call it directly to finalize the
> > job once the pointer is handed over. And I mean explicitly calling
> > qemuBlockJobStartupFinalize even in the success code path even when for
> > now it just unrefs the object.
>
> Not sure if I understand this correctly. Once I fix patch 2/30 to always
> call qemuBlockJobStartupFinalize() and the same behavior remains here as
>
> well were we return the job pointer or NULL I should also cleanup the
> job pointer in the caller of qemuBlockCommit() with
> qemuBlockJobStartupFinalize()? If that understanding is correct it
> doesn't make sense to me.
>
> If we get a NULL there is no job so nothing to do. If we get a pointer
> it was already updated by qemuBlockJobStarted() so
> qemuBlockJobStartupFinalize() would only do the unref part and nothing
> else. If qemuBlockJobStartupFinalize() should be only used as cleanup
> for qemuBlockJobData we should make it that way.
Thinking about it a bit more I think it's okay to use virObjectUnref
without an explicit call to qemuBlockJobStartupFinalize() in cases where
you are 100% sure that the job was started.
Thus in the patch makes qemuBlockCommit() return the job pointer you
will no longer call qemuBlockJobStartupFinalize() and the caller will
have to unref the returned job.
The same thing will need to happen here then.
Originally I wanted to insist that all of the job cleanup goes through
qemuBlockJobStartupFinalize() but that actually doesn't make much sense.
