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

[Eric Blake <eblake@redhat.com>]

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

On 10/20/2015 01:38 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]