Re: [libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[John Ferlan]

On 10/25/2017 05:54 PM, Dawid Zamirski wrote:
> On Wed, 2017-10-25 at 17:35 -0400, John Ferlan wrote:
>>
>> On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
>>> 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);
>>
>> But @data is used shortly after this and I don't see in the calling
>> tree
>> a corresponding UISession.Open* of some type or am I missing it in
>> some
>> called function?
>>
>>
>> The rest looks good - just need to know about this.  I can remove
>> before
>> pushing or make some other sort of simple adjustment.
> 
> 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
> 
Reviewed-by: John Ferlan 

John

(pushed now too)

>>
>> John
>>
>> (I'm at KVM Forum in Prague - so normal work schedule is a bit off)
>>
>>>  
>>>
>>>

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

Re: [libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[Dawid Zamirski]

On Wed, 2017-10-25 at 17:35 -0400, John Ferlan wrote:
> 
> On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
> > 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);
> 
> But @data is used shortly after this and I don't see in the calling
> tree
> a corresponding UISession.Open* of some type or am I missing it in
> some
> called function?
> 
> 
> The rest looks good - just need to know about this.  I can remove
> before
> pushing or make some other sort of simple adjustment.

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

> 
> John
> 
> (I'm at KVM Forum in Prague - so normal work schedule is a bit off)
> 
> >  
> > 
> > 

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

Re: [libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[John Ferlan]

On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
> 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);

But @data is used shortly after this and I don't see in the calling tree
a corresponding UISession.Open* of some type or am I missing it in some
called function?


The rest looks good - just need to know about this.  I can remove before
pushing or make some other sort of simple adjustment.

John

(I'm at KVM Forum in Prague - so normal work schedule is a bit off)

>  
>  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, );
> -if (!strUtf8)
> -return -1;
> -
> -if (virStrToLong_i(strUtf8, NULL, 10, ) < 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, );
> +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, );
> +
> +if (NS_FAILED(rc) || !vrdeInfo) {
> +VIR_WARN("Could not get VBox VM VRDEServerInfo, rc=%08x", rc);
> +goto cleanup;
> +}
> +
> +rc = vrdeInfo->vtbl->GetPort(vrdeInfo, );
> +
> +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 

[libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[Dawid Zamirski]

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, );
-if (!strUtf8)
-return -1;
-
-if (virStrToLong_i(strUtf8, NULL, 10, ) < 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, );
+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, );
+
+if (NS_FAILED(rc) || !vrdeInfo) {
+VIR_WARN("Could not get VBox VM VRDEServerInfo, rc=%08x", rc);
+goto cleanup;
+}
+
+rc = vrdeInfo->vtbl->GetPort(vrdeInfo, );
+
+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", );
-rc = VRDEServer->vtbl->GetVRDEProperty(VRDEServer, VRDEPortsKey, 
);
-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,
+   );
+
+if (NS_FAILED(rc)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+