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

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

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