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

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