Re: [Qemu-devel] [PULL 09/10] monitor: add query-vnc2 command

[Markus Armbruster]

Gerd Hoffmann <address@hidden> writes:

> Add new query vnc qmp command, for the lack of better ideas just name it
> "query-vnc2".  Changes over query-vnc:
>
>  * It returns a list of vnc servers, so multiple vnc server instances
>    are covered.
>  * Each vnc server returns a list of server sockets.  Followup patch
>    will use that to also report websockets.  In case we add support for
>    multiple server sockets server sockets (to better support ipv4+ipv6
>    dualstack) we can add them to the list too.

I guess we could shoehorn this into query-vnc by having it return an
anonymous union and use an optional parameter to select the alternative.
Too much trouble just to avoid an ugly name.

Call it query-vnc-servers?

> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  qapi-schema.json |  68 ++++++++++++++++++++++++++++
>  qmp-commands.hx  |   5 +++
>  ui/vnc.c         | 133 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 206 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 563b4ad..2d45d4c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -751,6 +751,63 @@
>             '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']} 
> }
>  
>  ##
> +# @VncPriAuth:

I don't personally mind abbreviations, but maybe somebody else does:
QAPI / QMP tends to spell things out, like VncPrimaryAuthentication.
Maybe just VncPrimaryAuth, since there's precedence for abbreviating
authentication that way.

> +#
> +# vnc primary authentication method.
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'VncPriAuth',
> +  'data': [ 'none', 'vnc', 'ra2', 'ra2ne', 'tight', 'ultra',
> +            'tls', 'vencrypt', 'sasl' ] }
> +
> +##
> +# @VncVencryptSubAuth:
> +#
> +# vnc sub authentication method with vencrypt.
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'VncVencryptSubAuth',
> +  'data': [ 'plain',
> +            'tls-none',  'x509-none',
> +            'tls-vnc',   'x509-vnc',
> +            'tls-plain', 'x509-plain',
> +            'tls-sasl',  'x509-sasl' ] }
> +
> +##
> +# @VncInfo2:
> +#
> +# Information about a vnc server
> +#
> +# @id: vnc server name.
> +#
> +# @server: A list of @VncBasincInfo describing all listening sockets.
> +#          The list can be empty (in case the vnc server is disabled).
> +#          It also may have multiple entries: normal + websocket,
> +#          possibly also ipv4 + ipv6 in the future.
> +#
> +# @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
> +#
> +# @vencrypt: #optional The vencrypt sub authentication type used by the 
> server,
> +#            only specified in case auth == vencrypt.
> +#
> +# @display: #optional The display device the vnc server is linked to.
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'VncInfo2',
> +  'data': { 'id'        : 'str',
> +            'server'    : ['VncBasicInfo'],
> +            'clients'   : ['VncClientInfo'],
> +            'auth'      : 'VncPriAuth',
> +            '*vencrypt' : 'VncVencryptSubAuth',
> +            '*display'  : 'str' } }
> +
> +##
>  # @query-vnc:
>  #
>  # Returns information about the current VNC server
> @@ -762,6 +819,17 @@
>  { 'command': 'query-vnc', 'returns': 'VncInfo' }
>  
>  ##
> +# @query-vnc2:
> +#
> +# Returns a list of vnc servers.  The list can be empty.
> +#
> +# Returns: @VncInfo

Returns: a list of @VncInfo2

> +#
> +# Since: 2.3
> +##
> +{ 'command': 'query-vnc2', 'returns': ['VncInfo2'] }
> +
> +##
>  # @SpiceBasicInfo
>  #
>  # The basic information for SPICE network connection

Schema looks good to me otherwise, but I'd like to hear Eric's opinion.

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 3348782..dec288a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2825,6 +2825,11 @@ EQMP
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_vnc,
>      },
> +    {
> +        .name       = "query-vnc2",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_vnc2,
> +    },
>  
>  SQMP
>  query-spice
> diff --git a/ui/vnc.c b/ui/vnc.c
> index d7c7865..e730059 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -456,6 +456,139 @@ out_error:
>      return NULL;
>  }
>  
> +static VncBasicInfoList *qmp_query_server_entry(int socket,
> +                                                VncBasicInfoList *prev)
> +{
> +    VncBasicInfoList *list;
> +    VncBasicInfo *info;
> +    struct sockaddr_storage sa;
> +    socklen_t salen = sizeof(sa);
> +    char host[NI_MAXHOST];
> +    char serv[NI_MAXSERV];
> +
> +    if (getsockname(socket, (struct sockaddr *)&sa, &salen) < 0 ||
> +        getnameinfo((struct sockaddr *)&sa, salen,
> +                    host, sizeof(host), serv, sizeof(serv),
> +                    NI_NUMERICHOST | NI_NUMERICSERV) < 0) {
> +        return prev;
> +    }
> +
> +    info = g_new0(VncBasicInfo, 1);
> +    info->host = g_strdup(host);
> +    info->service = g_strdup(serv);
> +    info->family = inet_netfamily(sa.ss_family);
> +
> +    list = g_new0(VncBasicInfoList, 1);
> +    list->value = info;
> +    list->next = prev;
> +    return list;
> +}
> +
> +static void qmp_query_auth(VncDisplay *vd, VncInfo2 *info)
> +{
> +    switch (vd->auth) {
> +    case VNC_AUTH_VNC:
> +        info->auth = VNC_PRI_AUTH_VNC;
> +        break;
> +    case VNC_AUTH_RA2:
> +        info->auth = VNC_PRI_AUTH_RA2;
> +        break;
> +    case VNC_AUTH_RA2NE:
> +        info->auth = VNC_PRI_AUTH_RA2NE;
> +        break;
> +    case VNC_AUTH_TIGHT:
> +        info->auth = VNC_PRI_AUTH_TIGHT;
> +        break;
> +    case VNC_AUTH_ULTRA:
> +        info->auth = VNC_PRI_AUTH_ULTRA;
> +        break;
> +    case VNC_AUTH_TLS:
> +        info->auth = VNC_PRI_AUTH_TLS;
> +        break;
> +    case VNC_AUTH_VENCRYPT:
> +        info->auth = VNC_PRI_AUTH_VENCRYPT;
> +#ifdef CONFIG_VNC_TLS
> +        info->has_vencrypt = true;
> +        switch (vd->subauth) {
> +        case VNC_AUTH_VENCRYPT_PLAIN:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_PLAIN;
> +            break;
> +        case VNC_AUTH_VENCRYPT_TLSNONE:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_TLS_NONE;
> +            break;
> +        case VNC_AUTH_VENCRYPT_TLSVNC:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_TLS_VNC;
> +            break;
> +        case VNC_AUTH_VENCRYPT_TLSPLAIN:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_TLS_PLAIN;
> +            break;
> +        case VNC_AUTH_VENCRYPT_X509NONE:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_X509_NONE;
> +            break;
> +        case VNC_AUTH_VENCRYPT_X509VNC:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_X509_VNC;
> +            break;
> +        case VNC_AUTH_VENCRYPT_X509PLAIN:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_X509_PLAIN;
> +            break;
> +        case VNC_AUTH_VENCRYPT_TLSSASL:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_TLS_SASL;
> +            break;
> +        case VNC_AUTH_VENCRYPT_X509SASL:
> +            info->vencrypt = VNC_VENCRYPT_SUB_AUTH_X509_SASL;
> +            break;
> +        default:
> +            info->has_vencrypt = false;
> +            break;
> +        }
> +#endif
> +        break;
> +    case VNC_AUTH_SASL:
> +        info->auth = VNC_PRI_AUTH_SASL;
> +        break;
> +    case VNC_AUTH_NONE:
> +    default:
> +        info->auth = VNC_PRI_AUTH_NONE;
> +        break;
> +    }

T-e-d-i-o-u-s :)

What about mapping vnc.h's authenticaton modes to the QAPI enumeration
constants with tables rather than switches?  Your choice.

If the schema let us specify the enumeration values, we could avoid the
mapping altogether.

> +}
> +
> +VncInfo2List *qmp_query_vnc2(Error **errp)
> +{
> +    VncInfo2List *item, *prev = NULL;
> +    VncInfo2 *info;
> +    VncDisplay *vd;
> +    DeviceState *dev;
> +
> +    QTAILQ_FOREACH(vd, &vnc_displays, next) {
> +        info = g_new0(VncInfo2, 1);
> +        info->id = g_strdup(vd->id);
> +        info->clients = qmp_query_client_list(vd);
> +        qmp_query_auth(vd, info);
> +        if (vd->dcl.con) {
> +            dev = DEVICE(object_property_get_link(OBJECT(vd->dcl.con),
> +                                                  "device", NULL));
> +            info->has_display = true;
> +            info->display = g_strdup(dev->id);
> +        }
> +        if (vd->lsock != -1) {
> +            info->server = qmp_query_server_entry(vd->lsock,
> +                                                  info->server);
> +        }
> +#ifdef CONFIG_VNC_WS
> +        if (vd->lwebsock != -1) {
> +            /* TODO */
> +        }
> +#endif
> +
> +        item = g_new0(VncInfo2List, 1);
> +        item->value = info;
> +        item->next = prev;
> +        prev = item;
> +    }
> +    return prev;
> +}
> +
>  /* TODO
>     1) Get the queue working for IO.
>     2) there is some weirdness when using the -S option (the screen is grey