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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]