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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list