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.
With that change:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
