The VBOX_SESSION_OPEN/CLOSE macros are only called in _vboxDomainSnapshotRestore and they are unflexible because: * assume the caller will have variable named "data" * can only create Write lock type As per above, it's not that hard to simply use the VBOX API directly. --- src/vbox/vbox_tmpl.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index dffeabde0..ce2ee9037 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 } static void @@ -323,7 +317,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); @@ -368,7 +362,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; } -- 2.14.2

From: Dawid Zamirski <dzrudy(a)gmail.com&gt; Originally autoport in vbox driver was setting the port to default value (3389) which caused multiple 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 arbitrary port range (3389-3689) to avoid that issue. --- src/vbox/vbox_tmpl.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index ce2ee9037..2b3f2e3eb 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -146,6 +146,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) { @@ -1595,15 +1598,21 @@ _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.autoport) + VBOX_UTF8_TO_UTF16(VBOX_RDP_AUTOPORT_RANGE, &VRDEPortsValue); + else + VRDEPortsValue = PRUnicharFromInt(data->pFuncs, + graphics->data.rdp.port); + rc = VRDEServer->vtbl->SetVRDEProperty(VRDEServer, VRDEPortsKey, VRDEPortsValue); VBOX_UTF16_FREE(VRDEPortsKey); -- 2.14.2

VirutalBox has a IVRDEServerInfo structure 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 | 134 +++++++++++++++++++++++++++++++----------- src/vbox/vbox_uniformed_api.h | 2 +- 3 files changed, 104 insertions(+), 35 deletions(-) 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 2b3f2e3eb..c7682f13c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -221,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 */ @@ -280,6 +257,54 @@ static virDomainState _vboxConvertState(PRUint32 state) } } + +static int +vboxGetActiveVRDEServerPort(ISession *session, IMachine *machine) +{ + nsresult rc; + PRInt32 port = -1; + IVRDEServerInfo *vrdeInfo = NULL; + IConsole *console = NULL; + + 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); + return -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); + session->vtbl->UnlockMachine(session); + + return port; +} + + static int _vboxDomainSnapshotRestore(virDomainPtr dom, IMachine *machine, @@ -1576,24 +1601,67 @@ _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 - available only when VM is running and has + * the VBOX extensions installed (without extenstions RDP server + * functionality is disabled) + */ + port = vboxGetActiveVRDEServerPort(data->vboxSession, machine); + if (port > 0) + graphics->data.rdp.port = port; + + /* 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; } + VBOX_UTF16_TO_UTF8(VRDEPortsValue, &portUtf8); + + if (portUtf8) { + /* does the string contain digits only */ + nmatches = virStringSearch(portUtf8, "(^[[:digit:]]+$)", 1, &matches); + + /* the port property is not numeric, then it must be a port range or + * port list or combination of the two, either way it's an autoport + */ + if (nmatches != 1) + graphics->data.rdp.autoport = true; + + /* no active port available, e.g. VM is powered off, try to get it from + * the property string + */ + if (port < 0) { + if (nmatches == 1 && virStrToLong_i(portUtf8, NULL, 10, &port) == 0) + graphics->data.rdp.port = port; + } + } + + 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); -- 2.14.2

On Wed, 2017-10-25 at 17:35 -0400, John Ferlan wrote: Yep this should be removed - it's a left over from my old internal patch [1], that I forgot to remove when preparing for upstream submission. It was originally preceded with OpenExisting (aka LockMachine) to get the IConsole - the new patch does it internally in the vboxGetActiveVRDEServerPort function. https://github.com/datto/libvirt/commit/a3cb830bfce10b1a614c18a6ac50783 45433d900#diff-747d3af65e7ac81a564b7cb4fcd01eb6R3516 Thank you, Dawid