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@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
-- 
2.35.1

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