[PATCH] qemu: domainjob: Allow free if cb is not set in qemuDomainObjClearJob

[Michal Prívozník]

On 3/2/22 13:33, Kristina Hanicova wrote:
> We should allow resetting and freeing qemuDomainJobObj even if
> 'cb' attribute is not set. This is theoretical for now, but the
> attribute must not be always set in the future, so early return
> would create memory leaks. It is sufficient to check if 'cb'
> exists before dereferencing it in qemuDomainObjClearJob() and
> also qemuDomainObjResetAsyncJob() as the latter is called from
> the former.
> 
> This commit partially reverts af16e754cd4efc3ca1.
> 
> Signed-off-by: Kristina Hanicova <khanicov at redhat.com>
> ---
>  src/qemu/qemu_domainjob.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index 3e73eba4ed..587c166d94 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
> @@ -239,8 +239,10 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObj *job)
>      job->abortJob = false;
>      VIR_FREE(job->error);
>      g_clear_pointer(&job->current, virDomainJobDataFree);
> -    job->cb->resetJobPrivate(job->privateData);
>      job->apiFlags = 0;
> +
> +    if (job->cb)
> +        job->cb->resetJobPrivate(job->privateData);
>  }
>  
>  int
> @@ -270,16 +272,15 @@ qemuDomainObjRestoreJob(virDomainObj *obj,
>  void
>  qemuDomainObjClearJob(qemuDomainJobObj *job)
>  {
> -    if (!job->cb)
> -        return;
> -
>      qemuDomainObjResetJob(job);
>      qemuDomainObjResetAsyncJob(job);
> -    g_clear_pointer(&job->privateData, job->cb->freeJobPrivate);
>      g_clear_pointer(&job->current, virDomainJobDataFree);
>      g_clear_pointer(&job->completed, virDomainJobDataFree);
>      virCondDestroy(&job->cond);
>      virCondDestroy(&job->asyncCond);
> +
> +    if (job->cb)
> +        g_clear_pointer(&job->privateData, job->cb->freeJobPrivate);
>  }
>  
>  bool

This makes sense, and I'd just merge it. But I've got a question. So
this alloews job->cb to be NULL. But there are other functions which
takze job->cb and call a callback directly: qemuDomainObjRestoreJob(),
qemuDomainObjInitJob(), qemuDomainObjPrivateXMLFormatJob() and possibly
others. Would it make sense to threat them the same as you're doing here?

Michal