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

[Peter Krempa]

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 at redhat.com>
> ---
>  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.