[libvirt] [PATCH] qemu: fix graphics port allocation

[Daniel P. Berrange]

On Tue, Feb 26, 2013 at 02:58:19PM +0100, Ján Tomko wrote:
> Right now, we allocate a port or a TLS port for SPICE if it's set to -1,
> even if autoport is off. But we only free them if autoport is on.
> 
> With autoport on, we only allocate TLS port if cfg->spiceTLS is set, but
> we free it even if it's not, leading to an error message.
> 
> This patch separates the autoport=yes|no XML option into autoport and
> tlsAutoport in virDomainGraphicsDef:
> if autoport is yes, we set them both to 1 (and vice versa)
> if either of the port numbers is -1, the corresponding autoport is set
> to 1 and it's used as a condition for acquiring/releasing the port.
> 
> For TLS ports, cfg->spiceTLS is checked before releasing the port as
> well.
> 
> Also check the port number before trying to release it - the vnc port
> will be 0, the spice port will be -1 or 0 if it hasn't been allocated
> yet (if qemuProcessStart fails before port allocation).
> ---
>  src/conf/domain_conf.c  | 17 +++++++++++------
>  src/conf/domain_conf.h  |  1 +
>  src/qemu/qemu_hotplug.c |  3 ++-
>  src/qemu/qemu_process.c | 26 ++++++++++++++------------
>  4 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b65e52a..cb27f82 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7083,8 +7083,10 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
>          }
>  
>          if ((autoport = virXMLPropString(node, "autoport")) != NULL) {
> -            if (STREQ(autoport, "yes"))
> +            if (STREQ(autoport, "yes")) {
>                  def->data.spice.autoport = 1;
> +                def->data.spice.tlsAutoport = 1;
> +            }
>              VIR_FREE(autoport);
>          }
>  
> @@ -7102,12 +7104,14 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
>              VIR_FREE(defaultMode);
>          }
>  
> -        if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) {
> -            /* Legacy compat syntax, used -1 for auto-port */
> +        /* Legacy compat syntax, used -1 for auto-port */
> +        if (def->data.spice.port == -1)
>              def->data.spice.autoport = 1;
> -        }
> +        if (def->data.spice.tlsPort == -1)
> +            def->data.spice.tlsAutoport = 1;
>  
> -        if (def->data.spice.autoport && (flags & VIR_DOMAIN_XML_INACTIVE)) {
> +        if (def->data.spice.autoport && def->data.spice.tlsAutoport &&
> +            (flags & VIR_DOMAIN_XML_INACTIVE)) {
>              def->data.spice.port = 0;
>              def->data.spice.tlsPort = 0;
>          }
> @@ -14201,7 +14205,8 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>                                def->data.spice.tlsPort);
>  
>          virBufferAsprintf(buf, " autoport='%s'",
> -                          def->data.spice.autoport ? "yes" : "no");
> +                          (def->data.spice.autoport &&
> +                          def->data.spice.tlsAutoport) ? "yes" : "no");
>  
>          if (listenAddr)
>              virBufferAsprintf(buf, " listen='%s'", listenAddr);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0828954..cc67716 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1356,6 +1356,7 @@ struct _virDomainGraphicsDef {
>              char *keymap;
>              virDomainGraphicsAuthDef auth;
>              unsigned int autoport :1;
> +            unsigned int tlsAutoport :1;
>              int channels[VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST];
>              int defaultMode; /* enum virDomainGraphicsSpiceChannelMode */
>              int image;

NACK to these changes to domain_conf. The bug is in the QEMU code,
so the fix should be done in the QEMU code without introducing
this extra 'tlsAutoport' field that isn't in the XML anywhere.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|