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

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;
+    } 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);
+
+    /* 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;
+
+    VBOX_UTF16_TO_UTF8(VRDEPortsValue, &portUtf8);
+
+    if (portUtf8) {
+        nmatches = virStringSearch(portUtf8, "(^[[:digit:]]+$)", 1, &matches);
+
+        /* 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;
+        }
+    }
+
+ 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list