Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> A future qapi patch will rework generated structs with a base
> class to be unboxed.  In preparation for that, change the code
> that allocates then populates an info struct to instead merely
> populate the fields of an info field passed in as a parameter.
> Add rudimentary Error handling for cases where the old code
> returned NULL; but as before, callers merely ignore errors for
> now.

Actually, the call chain rooted at vnc_qmp_event() does handle failure
before this patch.  It ignores the error *object* after the patch.

>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: (no v6-8): hoist from v5 33/46
> ---
>  ui/vnc.c | 52 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index d73966a..61af4ba 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -156,10 +156,11 @@ char *vnc_socket_remote_addr(const char *format, int 
> fd) {
>      return addr_to_string(format, &sa, salen);
>  }
>
> -static VncBasicInfo *vnc_basic_info_get(struct sockaddr_storage *sa,
> -                                        socklen_t salen)
> +static void vnc_basic_info_get(struct sockaddr_storage *sa,
> +                               socklen_t salen,
> +                               VncBasicInfo *info,
> +                               Error **errp)
>  {
> -    VncBasicInfo *info;
>      char host[NI_MAXHOST];
>      char serv[NI_MAXSERV];
>      int err;
> @@ -168,42 +169,44 @@ static VncBasicInfo *vnc_basic_info_get(struct 
> sockaddr_storage *sa,
>                             host, sizeof(host),
>                             serv, sizeof(serv),
>                             NI_NUMERICHOST | NI_NUMERICSERV)) != 0) {
> -        VNC_DEBUG("Cannot resolve address %d: %s\n",
> -                  err, gai_strerror(err));
> -        return NULL;
> +        error_setg(errp, "Cannot resolve address %d: %s",
> +                   err, gai_strerror(err));

Printing err is fine for a debug message.  Less so for an error message.
Drop it?

> +        return;
>      }
>
> -    info = g_malloc0(sizeof(VncBasicInfo));
>      info->host = g_strdup(host);
>      info->service = g_strdup(serv);
>      info->family = inet_netfamily(sa->ss_family);
> -    return info;
>  }

The function no longer "gets info", it merely initializes it.  Rename it
accordingly?  Gerd?

More of the same below.

>
> -static VncBasicInfo *vnc_basic_info_get_from_server_addr(int fd)
> +static void vnc_basic_info_get_from_server_addr(int fd, VncBasicInfo *info,
> +                                                Error **errp)
>  {
>      struct sockaddr_storage sa;
>      socklen_t salen;
>
>      salen = sizeof(sa);
>      if (getsockname(fd, (struct sockaddr*)&sa, &salen) < 0) {
> -        return NULL;
> +        error_setg_errno(errp, errno, "getsockname failed");
> +        return;
>      }
>
> -    return vnc_basic_info_get(&sa, salen);
> +    vnc_basic_info_get(&sa, salen, info, errp);
>  }
>
> -static VncBasicInfo *vnc_basic_info_get_from_remote_addr(int fd)
> +static void vnc_basic_info_get_from_remote_addr(int fd, VncBasicInfo *info,
> +                                                Error **errp)
>  {
>      struct sockaddr_storage sa;
>      socklen_t salen;
>
>      salen = sizeof(sa);
>      if (getpeername(fd, (struct sockaddr*)&sa, &salen) < 0) {
> -        return NULL;
> +        error_setg_errno(errp, errno, "getpeername failed");
> +        return;
>      }
>
> -    return vnc_basic_info_get(&sa, salen);
> +    vnc_basic_info_get(&sa, salen, info, errp);
>  }
>
>  static const char *vnc_auth_name(VncDisplay *vd) {
> @@ -256,13 +259,10 @@ static const char *vnc_auth_name(VncDisplay *vd) {
>  static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
>  {
>      VncServerInfo *info;
> -    VncBasicInfo *bi = vnc_basic_info_get_from_server_addr(vd->lsock);
> -    if (!bi) {
> -        return NULL;
> -    }
>
>      info = g_malloc(sizeof(*info));
> -    info->base = bi;
> +    info->base = g_malloc(sizeof(*info->base));
> +    vnc_basic_info_get_from_server_addr(vd->lsock, info->base, NULL);
>      info->has_auth = true;
>      info->auth = g_strdup(vnc_auth_name(vd));
>      return info;

Uh, doesn't this change the return value when getsockname() in
vnc_basic_info_get_from_server_addr() fails?

> @@ -291,11 +291,15 @@ static void vnc_client_cache_auth(VncState *client)
>
>  static void vnc_client_cache_addr(VncState *client)
>  {
> -    VncBasicInfo *bi = vnc_basic_info_get_from_remote_addr(client->csock);
> -
> -    if (bi) {
> -        client->info = g_malloc0(sizeof(*client->info));
> -        client->info->base = bi;
> +    Error *err = NULL;
> +    client->info = g_malloc0(sizeof(*client->info));
> +    client->info->base = g_malloc0(sizeof(*client->info->base));
> +    vnc_basic_info_get_from_remote_addr(client->csock, client->info->base,
> +                                        &err);
> +    if (err) {
> +        qapi_free_VncClientInfo(client->info);
> +        client->info = NULL;
> +        error_free(err);
>      }
>  }

Outside this patch's scope, but since I'm looking at the code already...

Here's the only caller of vnc_server_info_get():

    static void vnc_qmp_event(VncState *vs, QAPIEvent event)
    {
        VncServerInfo *si;

        if (!vs->info) {
            return;
        }
        g_assert(vs->info->base);

        si = vnc_server_info_get(vs->vd);
        if (!si) {
--->        return;
        }

        switch (event) {
        case QAPI_EVENT_VNC_CONNECTED:
            qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
            break;
        case QAPI_EVENT_VNC_INITIALIZED:
            qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
            break;
        case QAPI_EVENT_VNC_DISCONNECTED:
            qapi_event_send_vnc_disconnected(si, vs->info, &error_abort);
            break;
        default:
            break;
        }

        qapi_free_VncServerInfo(si);
    }

When vnc_server_info_get() fails, the event is dropped.  Why is that
okay?  Failure seems unlikely, though.

Here's the only caller of vnc_client_cache_addr():

    static void vnc_connect(VncDisplay *vd, int csock,
                            bool skipauth, bool websocket)
    {
        VncState *vs = g_malloc0(sizeof(VncState));
        int i;

[...]

--->    vnc_client_cache_addr(vs);
        vnc_qmp_event(vs, QAPI_EVENT_VNC_CONNECTED);
        vnc_set_share_mode(vs, VNC_SHARE_MODE_CONNECTING);

        if (!vs->websocket) {
            vnc_init_state(vs);
        }

        if (vd->num_connecting > vd->connections_limit) {
            QTAILQ_FOREACH(vs, &vd->clients, next) {
                if (vs->share_mode == VNC_SHARE_MODE_CONNECTING) {
                    vnc_disconnect_start(vs);
                    return;
                }
            }
        }
    }

vnc_client_cache_addr(vs) leaves vs->info null on failure.  Gives me a
queasy feeling...  The code in this file appears to cope with it.
Didn't check code elsewhere.