Originally autoport in vbox driver was setting the port to default
value (3389) which caused mutiple VM instances use the same port.
Since libvirt XML does not allow to set port ranges, this patch changes
the "autoport" behavior to set VBox's "TCP/Ports" property to an
arbitraty port range (3389-3689) to avoid that issue.
---
 src/vbox/vbox_tmpl.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index dffeabde0..8e47d90d6 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -152,6 +152,9 @@ if (strUtf16) {\
 
 #define VBOX_IID_INITIALIZER { NULL, true }
 
+/* default RDP port range to use for auto-port setting */
+#define VBOX_RDP_AUTOPORT_RANGE "3389-3689"
+
 static void
 _vboxIIDUnalloc(vboxDriverPtr data, vboxIID *iid)
 {
@@ -1601,20 +1604,27 @@ _vrdeServerGetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
 }
 
 static nsresult
-_vrdeServerSetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
-                    IVRDEServer *VRDEServer, virDomainGraphicsDefPtr graphics)
+_vrdeServerSetPorts(vboxDriverPtr data, IVRDEServer *VRDEServer,
+                    virDomainGraphicsDefPtr graphics)
 {
     nsresult rc = 0;
     PRUnichar *VRDEPortsKey = NULL;
     PRUnichar *VRDEPortsValue = NULL;
 
     VBOX_UTF8_TO_UTF16("TCP/Ports", &VRDEPortsKey);
-    VRDEPortsValue = PRUnicharFromInt(data->pFuncs, graphics->data.rdp.port);
+
+    if (graphics->data.rdp.port)
+        VRDEPortsValue = PRUnicharFromInt(data->pFuncs,
+                                          graphics->data.rdp.port);
+    else if (graphics->data.rdp.autoport)
+        VBOX_UTF8_TO_UTF16(VBOX_RDP_AUTOPORT_RANGE, &VRDEPortsValue);
+
     rc = VRDEServer->vtbl->SetVRDEProperty(VRDEServer, VRDEPortsKey,
                                            VRDEPortsValue);
     VBOX_UTF16_FREE(VRDEPortsKey);
     VBOX_UTF16_FREE(VRDEPortsValue);
 
+
     return rc;
 }
 
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/10/2017 05:52 PM, Dawid Zamirski wrote:
> Originally autoport in vbox driver was setting the port to default
> value (3389) which caused mutiple VM instances use the same port.
> Since libvirt XML does not allow to set port ranges, this patch changes
> the "autoport" behavior to set VBox's "TCP/Ports" property to an
> arbitraty port range (3389-3689) to avoid that issue.

arbitrary

> ---
>  src/vbox/vbox_tmpl.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index dffeabde0..8e47d90d6 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -152,6 +152,9 @@ if (strUtf16) {\
>  
>  #define VBOX_IID_INITIALIZER { NULL, true }
>  
> +/* default RDP port range to use for auto-port setting */
> +#define VBOX_RDP_AUTOPORT_RANGE "3389-3689"
> +
>  static void
>  _vboxIIDUnalloc(vboxDriverPtr data, vboxIID *iid)
>  {
> @@ -1601,20 +1604,27 @@ _vrdeServerGetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
>  }
>  
>  static nsresult
> -_vrdeServerSetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED,
> -                    IVRDEServer *VRDEServer, virDomainGraphicsDefPtr graphics)
> +_vrdeServerSetPorts(vboxDriverPtr data, IVRDEServer *VRDEServer,
> +                    virDomainGraphicsDefPtr graphics)
>  {
>      nsresult rc = 0;
>      PRUnichar *VRDEPortsKey = NULL;
>      PRUnichar *VRDEPortsValue = NULL;
>  
>      VBOX_UTF8_TO_UTF16("TCP/Ports", &VRDEPortsKey);
> -    VRDEPortsValue = PRUnicharFromInt(data->pFuncs, graphics->data.rdp.port);
> +
> +    if (graphics->data.rdp.port)

So one of my pet peeves in libvirt here and it's perhaps a latent or
day1 bug...

Looking through history - I'm not quite sure autoport ever quite worked
right because domain_conf would allow rdp.port == -1 in order to set
rdp.autoport = 1 (or true).

If rdp.port == -1, then this test passes which would set the "TCP/Ports"
property to -1. Now maybe that works, but I'm assuming it doesn't and an
error is thrown. Perhaps something you could test - modify the XML to
"<graphics type='rdp' ports='-1'/> and see what happens.

Since I went and dug a bit...  Looking at various commits in this space:

Commit to add version 4000 support: 8d2e24d6

Commit to add version 3001 support (< 3001, >= 3001) : 834d654

Original commit: 10d1650

It seems >= 3001 support totally ignored autoport setting


So my initial suggestion is alter the order of the checks.  That is:

   if (...rdp.autoport)
      set port range
   else
      set port to provided value

That way we won't have to worry about -1.

Also, please modify the formatdomain.html.in page in order to describe
that autoport will by default select a port in the range of 3389-3689.
Yes previously at some point in distant history 3389 was set to the
default because a 0 was allowed to be used to define that by the SetPort
API.

Secondary to that if you really feel a bit adventurous, you could add
attributes to the <graphics> element in order to define a port range
(min, max) in order to allow the autoport to select from outside your
default selection. Not required and hopefully 300 ports are enough, but
one never quite knows.

> +        VRDEPortsValue = PRUnicharFromInt(data->pFuncs,
> +                                          graphics->data.rdp.port);
> +    else if (graphics->data.rdp.autoport)
> +        VBOX_UTF8_TO_UTF16(VBOX_RDP_AUTOPORT_RANGE, &VRDEPortsValue);
> +
>      rc = VRDEServer->vtbl->SetVRDEProperty(VRDEServer, VRDEPortsKey,
>                                             VRDEPortsValue);
>      VBOX_UTF16_FREE(VRDEPortsKey);
>      VBOX_UTF16_FREE(VRDEPortsValue);
>  
> +

Spurious empty line

John
>      return rc;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list