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