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

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?

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

@@ -4111,12 +4116,12 @@ qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr cfg, memset(glisten, 0, sizeof(virDomainGraphicsListenDef)); if (virAsprintf(&glisten->socket, "%s/%s.sock", priv->libDir, type) < 0) - return -1; + goto cleanup; glisten->fromConfig = true; glisten->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET; } else if (listenAddr) { if (VIR_STRDUP(glisten->address, listenAddr) < 0) - return -1; + goto cleanup; glisten->fromConfig = true; } } @@ -4128,14 +4133,14 @@ qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr cfg,

if (qemuProcessGraphicsSetupNetworkAddress(glisten, listenAddr) < 0) - return -1; + goto cleanup; break;

case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET: if (!glisten->socket) { if (virAsprintf(&glisten->socket, "%s/%s.sock", priv->libDir, type) < 0) - return -1; + goto cleanup; glisten->autoGenerated = true; } break; @@ -4146,7 +4151,11 @@ qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr cfg, } }

- return 0; + ret = 0; + + cleanup: + virObjectUnref(cfg); + return ret; }

@@ -4155,7 +4164,6 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int flags) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool allocate = !(flags & VIR_QEMU_PROCESS_START_PRETEND); size_t i; int ret = -1; @@ -4176,8 +4184,7 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver, break;

case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: - if (qemuProcessSPICEAllocatePorts(driver, cfg, graphics, - allocate) < 0) + if (qemuProcessSPICEAllocatePorts(driver, graphics, allocate) < 0) goto cleanup; break;

@@ -4189,14 +4196,13 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver, } }

- if (qemuProcessGraphicsSetupListen(cfg, graphics, vm) < 0) + if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0) goto cleanup; }

ret = 0;

cleanup: - virObjectUnref(cfg); return ret; }