[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers |
Date: |
Tue, 20 Oct 2015 16:56:14 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 10/20/2015 01:38 AM, Markus Armbruster wrote:
> 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.
>
>> @@ -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
- [Qemu-devel] [PATCH v9 00/17] qapi collision reduction (post-introspection subset B'), Eric Blake, 2015/10/16
- [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Eric Blake, 2015/10/16
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Gerd Hoffmann, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Eric Blake, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/21
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Daniel P. Berrange, 2015/10/21
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/23
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers,
Eric Blake <=
[Qemu-devel] [PATCH v9 10/17] nbd: Convert to new qapi union layout, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 01/17] qapi: Add tests for reserved name abuse, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 08/17] tests: Convert to new qapi union layout, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' member names, Eric Blake, 2015/10/16