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

Re: [libvirt] [PATCH v4 06/14] graphics: resolve address for listen type network in qemu_process



On 05/19/2016 07:35 AM, Pavel Hrdina wrote:
> Both VNC and SPICE requires the same code to resolve address for listen
> type network.  Remove code duplication and create a new function that
> will be used in qemuProcessSetupGraphics().
> 
> Signed-off-by: Pavel Hrdina <phrdina redhat com>
> ---
>  src/qemu/qemu_command.c | 103 ++++++------------------------------------------
>  src/qemu/qemu_process.c |  47 +++++++++++++++++++++-
>  2 files changed, 57 insertions(+), 93 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e5847f7..ee43e21 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7384,10 +7384,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>  {
>      virBuffer opt = VIR_BUFFER_INITIALIZER;
>      virDomainGraphicsListenDefPtr glisten = NULL;
> -    const char *listenAddr = NULL;
> -    char *netAddr = NULL;
>      bool escapeAddr;
> -    int ret;
>  
>      if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC)) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -7395,6 +7392,8 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>          goto error;
>      }
>  
> +    glisten = virDomainGraphicsGetListen(graphics, 0);
> +
>      if (graphics->data.vnc.socket || cfg->vncAutoUnixSocket) {
>          if (!graphics->data.vnc.socket) {
>              if (virAsprintf(&graphics->data.vnc.socket,
> @@ -7416,52 +7415,15 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>              goto error;
>          }
>  
> -        if ((glisten = virDomainGraphicsGetListen(graphics, 0))) {
> -
> -            switch (glisten->type) {
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> -                listenAddr = glisten->address;
> -                break;
> -
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> -                if (!glisten->network)
> -                    break;
> -
> -                ret = networkGetNetworkAddress(glisten->network, &netAddr);
> -                if (ret <= -2) {
> -                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                                   "%s", _("network-based listen not possible, "
> -                                           "network driver not present"));
> -                    goto error;
> -                }
> -                if (ret < 0)
> -                    goto error;
> -
> -                listenAddr = netAddr;
> -                /* store the address we found in the <graphics> element so it
> -                 * will show up in status. */
> -                if (VIR_STRDUP(glisten->address, netAddr) < 0)
> -                    goto error;
> -                break;
> -
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> -                break;
> -            }
> +        if (glisten && glisten->address) {
> +            escapeAddr = strchr(glisten->address, ':') != NULL;
> +            if (escapeAddr)
> +                virBufferAsprintf(&opt, "[%s]", glisten->address);
> +            else
> +                virBufferAdd(&opt, glisten->address, -1);
>          }
> -
> -        if (!listenAddr)
> -            listenAddr = cfg->vncListen;
> -

This bit being dropped kinda confused me, but I see that this is taken care of
at the new SetupNetworkAddress callers already

> -        escapeAddr = strchr(listenAddr, ':') != NULL;
> -        if (escapeAddr)
> -            virBufferAsprintf(&opt, "[%s]", listenAddr);
> -        else
> -            virBufferAdd(&opt, listenAddr, -1);
>          virBufferAsprintf(&opt, ":%d",
>                            graphics->data.vnc.port - 5900);
> -
> -        VIR_FREE(netAddr);
>      }
>  
>      if (!graphics->data.vnc.socket &&
> @@ -7525,7 +7487,6 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>      return 0;
>  
>   error:
> -    VIR_FREE(netAddr);
>      virBufferFreeAndReset(&opt);
>      return -1;
>  }
> @@ -7539,9 +7500,6 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>  {
>      virBuffer opt = VIR_BUFFER_INITIALIZER;
>      virDomainGraphicsListenDefPtr glisten = NULL;
> -    const char *listenAddr = NULL;
> -    char *netAddr = NULL;
> -    int ret;
>      int defaultMode = graphics->data.spice.defaultMode;
>      int port = graphics->data.spice.port;
>      int tlsPort = graphics->data.spice.tlsPort;
> @@ -7553,6 +7511,8 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>          goto error;
>      }
>  
> +    glisten = virDomainGraphicsGetListen(graphics, 0);
> +
>      if (port > 0)
>          virBufferAsprintf(&opt, "port=%u,", port);
>  
> @@ -7567,46 +7527,8 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>      }
>  
>      if (port > 0 || tlsPort > 0) {
> -        if ((glisten = virDomainGraphicsGetListen(graphics, 0))) {
> -
> -            switch (glisten->type) {
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> -                listenAddr = glisten->address;
> -                break;
> -
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> -                if (!glisten->network)
> -                    break;
> -
> -                ret = networkGetNetworkAddress(glisten->network, &netAddr);
> -                if (ret <= -2) {
> -                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                                   "%s", _("network-based listen not possible, "
> -                                           "network driver not present"));
> -                    goto error;
> -                }
> -                if (ret < 0)
> -                    goto error;
> -
> -                listenAddr = netAddr;
> -                /* store the address we found in the <graphics> element so it will
> -                 * show up in status. */
> -                if (VIR_STRDUP(glisten->address, listenAddr) < 0)
> -                    goto error;
> -                break;
> -
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> -            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> -                break;
> -            }
> -        }
> -
> -        if (!listenAddr)
> -            listenAddr = cfg->spiceListen;
> -        if (listenAddr)
> -            virBufferAsprintf(&opt, "addr=%s,", listenAddr);
> -
> -        VIR_FREE(netAddr);
> +        if (glisten && glisten->address)
> +            virBufferAsprintf(&opt, "addr=%s,", glisten->address);
>      }
>  
>      if (cfg->spiceSASL) {
> @@ -7775,7 +7697,6 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>      return 0;
>  
>   error:
> -    VIR_FREE(netAddr);
>      virBufferFreeAndReset(&opt);
>      return -1;
>  }
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f4bf6c1..75c8e53 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4388,6 +4388,32 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
>  
>  
>  static int
> +qemuProcessGraphicsSetupNetworkAddress(virDomainGraphicsListenDefPtr glisten,
> +                                       const char *listenAddr)
> +{
> +    int rc;
> +
> +    if (!glisten->network) {
> +        if (VIR_STRDUP(glisten->address, listenAddr) < 0)
> +            return -1;
> +        return 0;
> +    }
> +

This is a logic change. Previously we accept this XML

<graphics ...>
  <listen type='network'/>
</graphics>

and silently treat that as using vnc_listen/spice_listen. Now we stick that
address in the XML like

<graphics ...>
  <listen type='network' address='$vnc_listen'/>
</graphics>

Which at least is more explicit, but it is a logic change. Just shows that the
address type='network' stuff needs more test coverage at least. I think at
some point we should reject bare type='network' if it doesn't have a @network
attribute

If that change was intentional it should be an additive patch after this
cleanup, with a test case

> +    rc = networkGetNetworkAddress(glisten->network, &glisten->address);
> +    if (rc <= -2) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("network-based listen isn't possible, "
> +                         "network driver isn't present"));
> +        return -1;
> +    }
> +    if (rc < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
> +static int
>  qemuProcessSetupGraphics(virQEMUDriverPtr driver,
>                           virDomainObjPtr vm,
>                           unsigned int flags)
> @@ -4429,12 +4455,29 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
>          for (j = 0; j < graphics->nListens; j++) {
>              virDomainGraphicsListenDefPtr glisten = &graphics->listens[j];
>  
> -            if (glisten->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS &&
> -                !glisten->address && listenAddr) {
> +            switch (glisten->type) {
> +            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> +                if (glisten->address || !listenAddr)
> +                    continue;
> +
>                  if (VIR_STRDUP(glisten->address, listenAddr) < 0)
>                      goto cleanup;
>  
>                  glisten->fromConfig = true;
> +                break;
> +
> +            case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> +                if (glisten->address || !listenAddr)
> +                    continue;
> +

Can listenAddr ever be NULL? I know the logic is pre-existing, but I think the
check is redundant. Particularly so for this case if the bit I mention above
is changed

- Cole


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