On 10/20/2015 01:38 AM, Markus Armbruster wrote: > 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. >
>> @@ -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?
Ah, as in "Cannot resolve address: %s", gai_strerror(err). Sure, sounds
okay to me.
>> @@ -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?
Hmm. I wrote the patch back in July (wow - review has been taking a
while...), don't know what I was thinking. Yes, I need to fix this to
return NULL in the same situations the pre-patch version did, or else
pass errp to the caller (looks like just one: vnc_qmp_event()). Or
maybe I was intentionally thinking that a best-effort result was
appropriate, particularly since the next patch gets rid of the base
member and therefore the possibility of info->base being NULL (maybe
that just means I rebased my series wrong when splitting one patch into
two).
Will fix.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
