[libvirt] [PATCH v4 06/14] graphics: resolve address for listen type network in qemu_process
Pavel Hrdina
phrdina at redhat.com
Fri May 20 07:59:43 UTC 2016
On Thu, May 19, 2016 at 04:50:24PM -0400, Cole Robinson wrote:
> 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 at 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;
[1]
> > -
> > - 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;
> > -
[2]
> 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>
This isn't a logic change, it only improves the live XML, so the only change here
is that the address will appear in the live XML. If you look at the old code
we've always used the config listen address if there wasn't any address resolved
for listen type network. If you look at the switch [1] we jump out from the
switch in case there is no network specified and we use the config address [2].
>
> 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
I agree that this configuration should be rejected, but I would wait for Peter's
patches about the validation domain XMLs.
>
> 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
You're probably right, QEMU supports VNC, SPICE and SDL but only VNC, SPICE and
RDP have listens so currently it cannot be NULL in this place, but I would
rather leave the check here just in case.
Pavel
More information about the libvir-list
mailing list