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@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?
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list