Re: [libvirt] [PATCH 1/5] qemu_process: graphics: ref driver config only in function where it is used

17 Aug 2016

On Tue, Aug 16, 2016 at 06:32:51PM -0400, John Ferlan wrote:
...
On 08/15/2016 07:28 AM, Pavel Hrdina wrote:
...
Signed-off-by: Pavel Hrdina <phrdina@redhat.com
---
 src/qemu/qemu_process.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)
ACK - couple of nits below to consider...
John
...

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7481626..26aef5e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3545,14 +3545,15 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
static int
 qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
-                              virQEMUDriverConfigPtr cfg,
                               virDomainGraphicsDefPtr graphics,
                               bool allocate)
 {
+    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     unsigned short port = 0;
     unsigned short tlsPort;
While not necessary yet, future adjustment proofing makes me wonder if
it's worth it to initialize this to 0 and the corresponding
virPortAllocatorRelease call added in cleanup?
Sure, this can be done as a follow up.
...
...
size_t i;
     int defaultMode = graphics->data.spice.defaultMode;
+    int ret = -1;
bool needTLSPort = false;
     bool needPort = false;
@@ -3598,12 +3599,13 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
         if (needTLSPort || graphics->data.spice.tlsPort == -1)
             graphics->data.spice.tlsPort = 5902;
-        return 0;
+        ret = 0;
+        goto cleanup;
     }
if (needPort || graphics->data.spice.port == -1) {
         if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
-            goto error;
+            goto cleanup;
graphics->data.spice.port = port;
@@ -3616,11 +3618,11 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("Auto allocation of spice TLS port requested "
                              "but spice TLS is disabled in qemu.conf"));
-            goto error;
+            goto cleanup;
         }
if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0)
-            goto error;
+            goto cleanup;
graphics->data.spice.tlsPort = tlsPort;
@@ -3628,11 +3630,12 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
             graphics->data.spice.tlsPortReserved = true;
     }
-    return 0;
+    ret = 0;
- error:
+ cleanup:
     virPortAllocatorRelease(driver->remotePorts, port);
-    return -1;
+    virObjectUnref(cfg);
+    return ret;
 }
@@ -4070,15 +4073,17 @@ qemuProcessGraphicsSetupNetworkAddress(virDomainGraphicsListenDefPtr glisten,
static int
-qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr cfg,
+qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver,
                                virDomainGraphicsDefPtr graphics,
                                virDomainObjPtr vm)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
+    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     const char *type = virDomainGraphicsTypeToString(graphics->type);
     char *listenAddr = NULL;
     bool useSocket = false;
     size_t i;
+    int ret = -1;
switch (graphics->type) {
     case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
"Technically" after this switch cfg isn't used, so it could be unref'd
avoiding the cleanup changes...
Well, yes that's true but as future proofing I would rather use cleanup in case
that someone uses *cfg* later on in this function.

Pavel

    