Re: [libvirt] [PATCH] graphics: Cleanup port policy

2012-03-12 Thread Eric Blake 
On 03/12/2012 10:06 AM, Michal Privoznik wrote:
> Even though we say in documentation setting (tls-)port to -1 is legacy
> compat style for enabling autoport, we're roughly doing this for VNC.
> However, in case of SPICE auto enable autoport iff both port & tlsPort
> are equal -1 as documentation says autoport plays with both.
> ---
>  src/conf/domain_conf.c  |   30 --
>  src/conf/domain_conf.h  |5 +
>  src/qemu/qemu_command.c |2 +-
>  src/qemu/qemu_process.c |   33 -
>  4 files changed, 46 insertions(+), 24 deletions(-)

ACK.  I had to look up the existing docs, which indeed state:

  Starts a SPICE server. The port attribute
  specifies the TCP port number (with -1 as legacy syntax
  indicating that it should be auto-allocated),
  while tlsPort gives an alternative secure
  port number. The autoport attribute is the
  new preferred syntax for indicating autoallocation of
  both port numbers.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] graphics: Cleanup port policy

2012-03-12 Thread Michal Privoznik 
Even though we say in documentation setting (tls-)port to -1 is legacy
compat style for enabling autoport, we're roughly doing this for VNC.
However, in case of SPICE auto enable autoport iff both port & tlsPort
are equal -1 as documentation says autoport plays with both.
---
 src/conf/domain_conf.c  |   30 --
 src/conf/domain_conf.h  |5 +
 src/qemu/qemu_command.c |2 +-
 src/qemu/qemu_process.c |   33 -
 4 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b185fe7..d142512 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5929,6 +5929,10 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
 VIR_FREE(port);
 goto error;
 }
+/* Legacy compat syntax, used -1 for auto-port */
+if (def->data.rdp.port == -1)
+def->data.rdp.autoport = 1;
+
 VIR_FREE(port);
 } else {
 def->data.rdp.port = 0;
@@ -5936,14 +5940,15 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
 }
 
 if ((autoport = virXMLPropString(node, "autoport")) != NULL) {
-if (STREQ(autoport, "yes")) {
-if (flags & VIR_DOMAIN_XML_INACTIVE)
-def->data.rdp.port = 0;
+if (STREQ(autoport, "yes"))
 def->data.rdp.autoport = 1;
-}
+
 VIR_FREE(autoport);
 }
 
+if (def->data.rdp.autoport && (flags & VIR_DOMAIN_XML_INACTIVE))
+def->data.rdp.port = 0;
+
 if ((replaceUser = virXMLPropString(node, "replaceUser")) != NULL) {
 if (STREQ(replaceUser, "yes")) {
 def->data.rdp.replaceUser = 1;
@@ -6009,16 +6014,21 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
 }
 
 if ((autoport = virXMLPropString(node, "autoport")) != NULL) {
-if (STREQ(autoport, "yes")) {
-if (flags & VIR_DOMAIN_XML_INACTIVE) {
-def->data.spice.port = 0;
-def->data.spice.tlsPort = 0;
-}
+if (STREQ(autoport, "yes"))
 def->data.spice.autoport = 1;
-}
 VIR_FREE(autoport);
 }
 
+if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) {
+/* Legacy compat syntax, used -1 for auto-port */
+def->data.spice.autoport = 1;
+}
+
+if (def->data.spice.autoport && (flags & VIR_DOMAIN_XML_INACTIVE)) {
+def->data.spice.port = 0;
+def->data.spice.tlsPort = 0;
+}
+
 def->data.spice.keymap = virXMLPropString(node, "keymap");
 
 if (virDomainGraphicsAuthDefParseXML(node, &def->data.spice.auth,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6fc307e..6da22f4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1183,6 +1183,11 @@ struct _virDomainGraphicsListenDef {
 };
 
 struct _virDomainGraphicsDef {
+/* Port value discipline:
+ * Value -1 is legacy syntax indicating that it should be auto-allocated.
+ * Value 0 means port wasn't specified in XML at all.
+ * Positive value is actual port number given in XML.
+ */
 int type;
 union {
 struct {
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 996763c..b6dd1f1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5375,7 +5375,7 @@ qemuBuildCommandLine(virConnectPtr conn,
 
 virBufferAsprintf(&opt, "port=%u", def->graphics[0]->data.spice.port);
 
-if (def->graphics[0]->data.spice.tlsPort) {
+if (def->graphics[0]->data.spice.tlsPort > 0) {
 if (!driver->spiceTLS) {
 qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 _("spice TLS port set in XML configuration,"
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1ac892f..ef311d1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3169,28 +3169,35 @@ int qemuProcessStart(virConnectPtr conn,
 goto cleanup;
 }
 vm->def->graphics[0]->data.vnc.port = port;
-} else if (vm->def->graphics[0]->type == 
VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
-   vm->def->graphics[0]->data.spice.autoport) {
-int port = qemuProcessNextFreePort(driver, QEMU_VNC_PORT_MIN);
-int tlsPort = -1;
-if (port < 0) {
-qemuReportError(VIR_ERR_INTERNAL_ERROR,
-"%s", _("Unable to find an unused SPICE 
port"));
-goto cleanup;
+} else if (vm->def->graphics[0]->type == 
VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
+int port = -1;
+if (vm->def->graphics[0]->data.spice.autoport ||
+vm->def->graphics[0]->data.spice.port == -1) {
+port = qemuProcessNext