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

[Pavel Hrdina]

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