From 0a9cb29ba257184efe409e7409cd474c07db9c0d Mon Sep 17 00:00:00 2001
From: Michal Privoznik <mprivozn@redhat.com>
Date: Wed, 27 Oct 2021 13:38:22 +0200
Subject: [PATCH] qemuAgentOpen: Rework domain object refcounting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently, when opening an agent socket the qemuConnectAgent()
increments domain object refcounter and calls qemuAgentOpen()
where the domain object pointer is simply stored inside
_qemuAgent struct. If qemuAgentOpen() fails, then it clears @cb
member only to avoid qemuProcessHandleAgentDestroy() being called
(which decrements the domain object refcounter) and the domain
object refcounter is then decreased explicitly in
qemuConnectAgent().

The same result can be achieved with much cleaner code: increment
the refcounter inside qemuAgentOpen() and drop the dance around
@cb.

Also, the comment in qemuConnectAgent() about holding an extra
reference is not correct. The thread that called
qemuConnectAgent() already holds a reference to the domain
object. No matter how many time the object is locked and unlocked
the reference counter can't be decreased.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_agent.c   | 14 +++++---------
 src/qemu/qemu_process.c |  7 -------
 2 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index d19a8b983d..e6d9e7ac50 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -160,9 +160,11 @@ qemuAgentEscapeNonPrintable(const char *text)
 static void qemuAgentDispose(void *obj)
 {
     qemuAgent *agent = obj;
+
     VIR_DEBUG("agent=%p", agent);
-    if (agent->cb && agent->cb->destroy)
-        (agent->cb->destroy)(agent, agent->vm);
+
+    if (agent->vm)
+        virObjectUnref(agent->vm);
     virCondDestroy(&agent->notify);
     g_free(agent->buffer);
     g_main_context_unref(agent->context);
@@ -671,7 +673,7 @@ qemuAgentOpen(virDomainObj *vm,
         virObjectUnref(agent);
         return NULL;
     }
-    agent->vm = vm;
+    agent->vm = virObjectRef(vm);
     agent->cb = cb;
     agent->singleSync = singleSync;
 
@@ -707,12 +709,6 @@ qemuAgentOpen(virDomainObj *vm,
     return agent;
 
  cleanup:
-    /* We don't want the 'destroy' callback invoked during
-     * cleanup from construction failure, because that can
-     * give a double-unref on virDomainObj *in the caller,
-     * so kill the callbacks now.
-     */
-    agent->cb = NULL;
     qemuAgentClose(agent);
     return NULL;
 }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 6e3d3b82e0..3623cf2535 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -234,19 +234,12 @@ qemuConnectAgent(virQEMUDriver *driver, virDomainObj *vm)
         goto cleanup;
     }
 
-    /* Hold an extra reference because we can't allow 'vm' to be
-     * deleted while the agent is active */
-    virObjectRef(vm);
-
     agent = qemuAgentOpen(vm,
                           config->source,
                           virEventThreadGetContext(priv->eventThread),
                           &agentCallbacks,
                           virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE));
 
-    if (agent == NULL)
-        virObjectUnref(vm);
-
     if (!virDomainObjIsActive(vm)) {
         qemuAgentClose(agent);
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",