Re: [Qemu-devel] [PATCH 8/8] ui: add ability to specify multiple VNC listen addresses

[Eric Blake <eblake@redhat.com>]

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

On 01/05/2017 10:07 AM, Daniel P. Berrange wrote:
> This change allows the listen address and websocket address
> options for -vnc to be repeated. This causes the VNC server
> to listen on multiple addresses. e.g.
> 
>  $ $QEMU -vnc vnc=localhost:1,vnc=unix:/tmp/vnc,\
>               websocket=127.0.0.1:8080,websocket=[::]:8081
> 
> results in listening on
> 
> 127.0.0.1:5901, 127.0.0.1:8080, ::1:5901, :::8081 & /tmp/vnc
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  ui/vnc.c | 191 +++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 130 insertions(+), 61 deletions(-)
> 

> +    for (i = 0; i < nsaddrstr; i++) {
> +        int rv;
> +        rv = vnc_display_get_address(saddrstr[i], false, 0, to,
> +                                     has_ipv4, has_ipv6,
> +                                     ipv4, ipv6,
> +                                     &saddr, errp);
> +        if (rv < 0) {
> +            goto cleanup;
> +        }
> +        /* Historical compat - if only a single listen address was
> +         * provided, then the display num can be used to set the
> +         * default websocket port
> +         */
> +        if (nsaddrstr == 1) {
> +            displaynum = rv;
> +        }
> +        *retsaddr = g_renew(SocketAddress *, *retsaddr, *retnsaddr + 1);
> +        (*retsaddr)[(*retnsaddr)++] = saddr;

Again, do we care if the user supplies LOTS of vnc addresses and
witnesses O(n^2) initialization cost?

>      }
> -    if (wsaddrstr) {
> -        if (vnc_display_get_address(wsaddrstr, true, displaynum, to,
> +    for (i = 0; i < nwsaddrstr; i++) {
> +        if (vnc_display_get_address(wsaddrstr[i], true, displaynum, to,
>                                      has_ipv4, has_ipv6,

> +
> +        *retwsaddr = g_renew(SocketAddress *, *retwsaddr, *retnwsaddr + 1);
> +        (*retwsaddr)[(*retnwsaddr)++] = wsaddr;

Same for websocket addresses.

>  static int vnc_display_connect(VncDisplay *vd,
> -                               SocketAddress *saddr,
> -                               SocketAddress *wsaddr,
> +                               SocketAddress **saddr,
> +                               size_t nsaddr,
> +                               SocketAddress **wsaddr,
> +                               size_t nwsaddr,
>                                 Error **errp)
>  {
>      /* connect to viewer */
>      QIOChannelSocket *sioc = NULL;
> -    if (wsaddr) {
> +    if (nwsaddr != 0) {
>          error_setg(errp, "Cannot use websockets in reverse mode");
>          return -1;
>      }
> -    vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
> +    if (nsaddr != 1) {
> +        error_setg(errp, "Expected a single address in reverse mo");

s/mo/mode/


>  static int vnc_display_listen(VncDisplay *vd,

> -    if (wsaddr &&
> -        vnc_display_listen_addr(vd, wsaddr,
> -                                "vnc-ws-listen",
> -                                &vd->lwebsock,
> -                                &vd->lwebsock_tag,
> -                                &vd->nlwebsock,
> -                                errp) < 0) {
> -        return -1;
> +    for (i = 0; i < nwsaddr; i++) {
> +        if (vnc_display_listen_addr(vd, wsaddr[i],
> +                                    "vnc-ws-listen",
> +                                    &vd->lwebsock,
> +                                    &vd->lwebsock_tag,
> +                                    &vd->nlwebsock,
> +                                    errp) < 0) {

And while your earlier arguments that resolving a single name into
multiple websocket IP addrs is not going to notice an O(n^2) growth
pattern, now we are calling that growth pattern over N websocket names
where N is user-controlled.  But this time I don't have a good bound on
how many IP addrs nlwebsock will grow to. So maybe we do want to
consider geometric allocation growth (requires tracking and passing an
allocated size alongside the currently-used size).

But overall I think you're enhancement makes sense.

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