[libvirt] [PATCH 1/3] vbox: Make autoport set RDP port range.

[John Ferlan]

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;
>  }
>  
>