Re: [Qemu-devel] [PATCH] qapi: Adding websocket information inside VncInfo structure.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3099 bytes --]

On 10/16/2015 10:37 AM, Julio Faracco wrote:
> When the user setup a vnc server to QEMU using the argument "-vnc
> 0.0.0.0:0,websocket=5701" for example, he cannot get information about which
> websocket port VNC is running. The command "info vnc" shows only details about
> the main VNC server and the clients. You can confirm that a websocket service
> is running, checking the ports that are opened. This commit introduce two new
> fields inside "VncInfo" either: "webservice" which contains the port as a
> string and the boolean value "has_websocket" that copies the status of
> ws_enabled from VNC Display.
> 
> Fixes #1486278
> 
> Signed-Off-By: Julio Faracco <jcfaracco@gmail.com>
> ---
>  hmp.c            |  3 +++
>  qapi-schema.json |  3 ++-
>  ui/vnc.c         | 25 +++++++++++++++++++++++++
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 

The choice of name 'has_websocket' won't work; it is forbidden by my
pending qapi patch:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03733.html

> diff --git a/hmp.c b/hmp.c
> index 5048eee..3fc74e0 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -559,6 +559,9 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>      if (info->has_host && info->has_service) {
>          monitor_printf(mon, "     address: %s:%s\n", info->host, info->service);
>      }
> +    if(info->has_websocket) {

Missing space after if.

> +        monitor_printf(mon, "   websocket: %s:%s\n", info->host, info->webservice);
> +    }

But you don't need has_websocket in the first place. Since you declared
webservice as optional in qapi, this should be:

if (info->has_webservice) {
   ... use info->webservice
}

>      if (info->has_auth) {
>          monitor_printf(mon, "        auth: %s\n", info->auth);
>      }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a386605..e97f78f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -916,7 +916,8 @@
>  { 'struct': 'VncInfo',
>    'data': {'enabled': 'bool', '*host': 'str',
>             '*family': 'NetworkAddressFamily',
> -           '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']} }
> +           '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo'],
> +           '*webservice': 'str', 'has_websocket': 'bool'} }

Missing documentation on the new optional webservice; it should look like:

# @webservice: #optional string that means something... (since 2.5)

and as argued above, I don't think you want has_websocket in the user
interface. The fact that webservice is optional is sufficient - if it is
present in QMP, then there is a web service; if it is absent, then
either this qemu is too old to have the schema support, or there is no
webservice.  And if qemu is new enough for introspection, then using
query-qmp-schema will tell us whether qemu was new enough to support it
and therefore that it is intentionally omitted because there is no
webservice.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]