Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> > > --- > 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.