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

On 10/10/2017 05:52 PM, Dawid Zamirski wrote:

VirutalBox has a IVRDEServerInfo sturcture available that gives the effective runtime port that the VM is using when it's running. This is useful when the "TCP/Ports" VBox property was set to port range (e.g. via autoport = "yes" or via VBoxManage) in which case it would be impossible to get the "active" port otherwise. --- src/vbox/vbox_common.c | 3 +- src/vbox/vbox_tmpl.c | 135 +++++++++++++++++++++++++++++------------- src/vbox/vbox_uniformed_api.h | 2 +- 3 files changed, 97 insertions(+), 43 deletions(-)

Is there more than one change going on here? The removal of VBOX_SESSION_{OPEN|CLOSE} could seemingly be a separately explained patch...

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 92ee37164..d542f2b76 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3326,7 +3326,8 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (VIR_ALLOC(graphics) < 0) goto cleanup;

- gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, graphics); + gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, machine, graphics); + gVBoxAPI.UISession.Close(data->vboxSession);

graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP;

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 8e47d90d6..ff69cf39c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -144,12 +144,6 @@ if (strUtf16) {\ (unsigned)(iid)->m3[7]);\ }\

-#define VBOX_SESSION_OPEN(/* unused */ iid_value, /* in */ machine) \ - machine->vtbl->LockMachine(machine, data->vboxSession, LockType_Write) - -#define VBOX_SESSION_CLOSE() \ - data->vboxSession->vtbl->UnlockMachine(data->vboxSession) - #define VBOX_IID_INITIALIZER { NULL, true }

/* default RDP port range to use for auto-port setting */ @@ -227,29 +221,6 @@ _vboxIIDFromArrayItem(vboxDriverPtr data, vboxIID *iid, _vboxIIDFromArrayItem(data, iid, array, idx) #define DEBUGIID(msg, strUtf16) DEBUGPRUnichar(msg, strUtf16)

-/** - * Converts Utf-16 string to int - */ -static int PRUnicharToInt(PCVBOXXPCOM pFuncs, PRUnichar *strUtf16) -{ - char *strUtf8 = NULL; - int ret = 0; - - if (!strUtf16) - return -1; - - pFuncs->pfnUtf16ToUtf8(strUtf16, &strUtf8); - if (!strUtf8) - return -1; - - if (virStrToLong_i(strUtf8, NULL, 10, &ret) < 0) - ret = -1; - - pFuncs->pfnUtf8Free(strUtf8); - - return ret; -} - /** * Converts int to Utf-16 string */ @@ -286,6 +257,56 @@ static virDomainState _vboxConvertState(PRUint32 state) } }

+static int +vboxGetActiveVRDEServerPort(ISession *session, IMachine *machine) +{ + nsresult rc; + PRInt32 port = -1; + IVRDEServerInfo *vrdeInfo = NULL; + IConsole *console = NULL; + int locked = 0; + + rc = machine->vtbl->LockMachine(machine, session, LockType_Shared); + if (NS_FAILED(rc)) { + VIR_WARN("Could not obtain shared lock on VBox VM, rc=%08x", rc); + goto cleanup;

Why not just return -1 here since cleanup wouldn't need to clean up @console or @vrdeInfo That way @locked isn't necessary either.

+ } else { + locked = 1; + } + + rc = session->vtbl->GetConsole(session, &console); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get VBox session console, rc=%08x", rc); + goto cleanup; + } + + /* it may be null if VM is not running */ + if (!console) + goto cleanup; + + rc = console->vtbl->GetVRDEServerInfo(console, &vrdeInfo); + + if (NS_FAILED(rc) || !vrdeInfo) { + VIR_WARN("Could not get VBox VM VRDEServerInfo, rc=%08x", rc); + goto cleanup; + } + + rc = vrdeInfo->vtbl->GetPort(vrdeInfo, &port); + + if (NS_FAILED(rc)) { + VIR_WARN("Could not read port from VRDEServerInfo, rc=%08x", rc); + goto cleanup; + } + + cleanup: + VBOX_RELEASE(console); + VBOX_RELEASE(vrdeInfo); + if (locked) + session->vtbl->UnlockMachine(session); + + return port; +} + static int _vboxDomainSnapshotRestore(virDomainPtr dom, IMachine *machine, @@ -326,7 +347,7 @@ _vboxDomainSnapshotRestore(virDomainPtr dom, goto cleanup; }

- rc = VBOX_SESSION_OPEN(domiid.value, machine); + rc = machine->vtbl->LockMachine(machine, data->vboxSession, LockType_Write); #if VBOX_API_VERSION < 5000000 if (NS_SUCCEEDED(rc)) rc = data->vboxSession->vtbl->GetConsole(data->vboxSession, &console); @@ -371,7 +392,7 @@ _vboxDomainSnapshotRestore(virDomainPtr dom, #if VBOX_API_VERSION < 5000000 VBOX_RELEASE(console); #endif /*VBOX_API_VERSION < 5000000*/ - VBOX_SESSION_CLOSE(); + data->vboxSession->vtbl->UnlockMachine(data->vboxSession); vboxIIDUnalloc(&domiid); return ret; } @@ -1582,24 +1603,56 @@ _vrdeServerSetEnabled(IVRDEServer *VRDEServer, PRBool enabled) }

static nsresult -_vrdeServerGetPorts(vboxDriverPtr data ATTRIBUTE_UNUSED, - IVRDEServer *VRDEServer, virDomainGraphicsDefPtr graphics) +_vrdeServerGetPorts(vboxDriverPtr data, IVRDEServer *VRDEServer, + IMachine *machine, virDomainGraphicsDefPtr graphics) { nsresult rc; PRUnichar *VRDEPortsKey = NULL; PRUnichar *VRDEPortsValue = NULL; + PRInt32 port = -1; + ssize_t nmatches = 0; + char **matches = NULL; + char *portUtf8 = NULL;

+ /* get active (effective) port, if available */ + port = vboxGetActiveVRDEServerPort(data->vboxSession, machine);

So if this returns -1, then ???

+ + /* get the port (or port range) set in VM properties, this info will + * be used to determine whether to set autoport flag + */ VBOX_UTF8_TO_UTF16("TCP/Ports", &VRDEPortsKey); - rc = VRDEServer->vtbl->GetVRDEProperty(VRDEServer, VRDEPortsKey, &VRDEPortsValue); - VBOX_UTF16_FREE(VRDEPortsKey); - if (VRDEPortsValue) { - /* even if vbox supports mutilpe ports, single port for now here */ - graphics->data.rdp.port = PRUnicharToInt(data->pFuncs, VRDEPortsValue); - VBOX_UTF16_FREE(VRDEPortsValue); - } else { - graphics->data.rdp.autoport = true; + rc = VRDEServer->vtbl->GetVRDEProperty(VRDEServer, VRDEPortsKey, + &VRDEPortsValue); + + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to read RDP port value, rc=%08x"), + (unsigned) rc); + goto cleanup; }

+ graphics->data.rdp.port = port;

!!? perhaps only set if port > 0 Would be overwritten later if @portUtf8... maybe this should be the else for "if (portUtf8)"? as long as port > 0; otherwise, we leave rdp.port as 0 and thus I'm guessing correctly port 3389 would be used. Just not sure that setting port = -1 if vboxGetActiveVRDEServerPort fails is the best thing

+ + VBOX_UTF16_TO_UTF8(VRDEPortsValue, &portUtf8); + + if (portUtf8) { + nmatches = virStringSearch(portUtf8, "(^[[:digit:]]+$)", 1, &matches);

I'm always astounded by regex's... Of course I also have no idea what VRDEPortsValue could return and thus cannot help whether this regex is right.

+ + /* vbox port-range -> autoport */ + if (nmatches != 1) { + graphics->data.rdp.autoport = true; + } else if (port < 0 && virStrToLong_i(portUtf8, NULL, 10, &port) == 0) { + /* no active port available but a single port was set in properties */ + graphics->data.rdp.port = port; + } + }

BTW: The old logic leaves me with the impression that it's possible to call SetVRDEProperty with a 0 as VRDEPortsValue and that is what autoport was "defined as". IOW: As you pointed out in your commit msg from the previous patch. John (light sometimes dawns on marblehead after he sends responses to previous patches and reads future patches).

+ + cleanup: + virStringListFree(matches); + VBOX_UTF8_FREE(portUtf8); + VBOX_UTF16_FREE(VRDEPortsValue); + VBOX_UTF16_FREE(VRDEPortsKey); + return rc; }

diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h index 2ccaf43e8..8cf27789b 100644 --- a/src/vbox/vbox_uniformed_api.h +++ b/src/vbox/vbox_uniformed_api.h @@ -341,7 +341,7 @@ typedef struct { nsresult (*GetEnabled)(IVRDEServer *VRDEServer, PRBool *enabled); nsresult (*SetEnabled)(IVRDEServer *VRDEServer, PRBool enabled); nsresult (*GetPorts)(vboxDriverPtr driver, IVRDEServer *VRDEServer, - virDomainGraphicsDefPtr graphics); + IMachine *machine, virDomainGraphicsDefPtr graphics); nsresult (*SetPorts)(vboxDriverPtr driver, IVRDEServer *VRDEServer, virDomainGraphicsDefPtr graphics); nsresult (*GetReuseSingleConnection)(IVRDEServer *VRDEServer, PRBool *enabled);