Re: [Qemu-devel] [PATCH 2/8] ui: fix reporting of VNC auth in query-vnc-servers

[Eric Blake <eblake@redhat.com>]

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

On 01/05/2017 10:06 AM, Daniel P. Berrange wrote:
> Currently the VNC authentication info is emitted at the
> top level of the query-vnc-servers data. This is wrong
> because the authentication scheme differs between plain
> and websockets when TLS is enabled. We should instead
> report auth against the individual servers. e.g.
> 
> (QEMU) query-vnc-servers
> {
>     "return": [
>         {
>             "clients": [],
>             "id": "default",
>             "auth": "vencrypt",

So we have to keep the old location for back-compat,...

>             "vencrypt": "x509-vnc",
>             "server": [
>                 {
>                     "host": "127.0.0.1"
>                     "service": "5901",
>                     "websocket": false,
>                     "family": "ipv4",
>                     "auth": "vencrypt",
>                     "vencrypt": "x509-vnc",

No trailing comma (JSON is too picky)

>                 }
>                 {
>                     "host": "127.0.0.1",
>                     "service": "5902",
>                     "websocket": true,
>                     "family": "ipv4",
>                     "auth": "vnc"

...but add the new location to fix the problem.
>                 },

No trailing comma

>             ]
>         }
>     ]
> }
> 
> This also future proofs the QMP schema so that we can
> cope with multiple VNC server instances, listening on
> different interfaces or ports, with different auth
> setup.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qapi-schema.json | 26 +++++++++++++++--
>  ui/vnc.c         | 85 +++++++++++++++++++++++++++++++++-----------------------
>  2 files changed, 73 insertions(+), 38 deletions(-)
> 

>  
> +
> +##
> +# @VncServerInfo2
> +#
> +# The network connection information for server
> +#
> +# @auth: The current authentication type used by the server
> +#
> +# @vencrypt: #optional The vencrypt sub authentication type used by the server,
> +#            only specified in case auth == vencrypt.
> +#
> +# Since: 2.8

2.9, actually

> @@ -1210,7 +1230,7 @@
>  # @clients: A list of @VncClientInfo of all currently connected clients.
>  #           The list can be empty, for obvious reasons.
>  #
> -# @auth: The current authentication type used by the server
> +# @auth: The current authentication type used by the non-websockets server

Since server is an array, should this read 'servers' or even 'server(s)'?

>  #
>  # @vencrypt: #optional The vencrypt sub authentication type used by the server,
>  #            only specified in case auth == vencrypt.
> @@ -1221,7 +1241,7 @@
>  ##
>  { 'struct': 'VncInfo2',
>    'data': { 'id'        : 'str',
> -            'server'    : ['VncBasicInfo'],
> +            'server'    : ['VncServerInfo2'],

Backwards-compatible, so you're fine making the change.

Once the nits are resolved, you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]