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(a)redhat.com&gt; > --- > 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