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