Re: [libvirt] [PATCH 1/3] vbox: Make autoport set RDP port range.
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;
}