[libvirt PATCH 02/30] qemu_block: extract block commit code to separate function

[Peter Krempa]

On Thu, Dec 08, 2022 at 14:30:38 +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_block.c  | 179 +++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_block.h  |   9 +++
>  src/qemu/qemu_driver.c | 162 +------------------------------------
>  3 files changed, 189 insertions(+), 161 deletions(-)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 8a6f601b29..4cca7555f3 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -3196,3 +3196,182 @@ qemuBlockExportAddNBD(virDomainObj *vm,

[...]

> +    if (rc < 0)
> +        goto error;
> +
> +    if (mirror) {
> +        disk->mirror = g_steal_pointer(&mirror);
> +        disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT;
> +    }
> +    qemuBlockJobStarted(job, vm);

[1]

> +
> +    return 0;
> +
> + error:
> +    if (clean_access) {
> +        virErrorPtr orig_err;
> +        virErrorPreserveLast(&orig_err);
> +        /* Revert access to read-only, if possible.  */
> +        qemuDomainStorageSourceAccessAllow(driver, vm, baseSource,
> +                                           true, false, false);
> +        if (top_parent && top_parent != disk->src)
> +            qemuDomainStorageSourceAccessAllow(driver, vm, top_parent,
> +                                               true, false, false);
> +
> +        virErrorRestore(&orig_err);
> +    }
> +    qemuBlockJobStartupFinalize(vm, job);

[2]

> +
> +    return -1;
> +}

[...]


> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d509582719..2f05da3d8c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

[...]

> -
> -    ret = qemuMonitorBlockCommit(priv->mon,
> -                                 qemuDomainDiskGetTopNodename(disk),
> -                                 job->name,
> -                                 topSource->nodeformat,
> -                                 baseSource->nodeformat,
> -                                 backingPath, speed);
> -
> -    qemuDomainObjExitMonitor(vm);
> -
> -    if (ret < 0)
> -        goto endjob;
> -
> -    if (mirror) {
> -        disk->mirror = g_steal_pointer(&mirror);
> -        disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT;
> -    }
> -    qemuBlockJobStarted(job, vm);
> +    ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, bandwidth, flags);
>  
>   endjob:
> -    if (ret < 0 && clean_access) {
> -        virErrorPtr orig_err;
> -        virErrorPreserveLast(&orig_err);
> -        /* Revert access to read-only, if possible.  */
> -        qemuDomainStorageSourceAccessAllow(driver, vm, baseSource,
> -                                           true, false, false);
> -        if (top_parent && top_parent != disk->src)
> -            qemuDomainStorageSourceAccessAllow(driver, vm, top_parent,
> -                                               true, false, false);
> -
> -        virErrorRestore(&orig_err);
> -    }
> -    qemuBlockJobStartupFinalize(vm, job);

So before this patch qemuBlockJobStartupFinalize() gets called both on
error and success code paths. After this patch [1] the success code path
doesn't call it any more, just the error code path [2].

Thus the reference to 'job' is leaked in the new code.

The new function must also call qemuBlockJobStartupFinalize() in both
cases.