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 :|