Re: [libvirt PATCH 10/30] qemu_block: change qemuBlockCommit to return job pointer

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(a)redhat.com&gt; > > --- > > 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(a)redhat.com&gt;