On 08/15/2016 07:28 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx
> ---
> 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;
> }
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list