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(a)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...