[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:53:24 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 10/20/2015 08:46 AM, Markus Armbruster wrote:
> Gerd Hoffmann <address@hidden> writes:
>
>> Hi,
>>
>>>> -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)
>>
>>> The function no longer "gets info", it merely initializes it. Rename it
>>> accordingly? Gerd?
>>
>> vnc_fill_basic_info maybe?
>
> Fine with me. Could also call it _init_ instead of _fill_.
I like init a bit better than fill.
>
>>> Outside this patch's scope, but since I'm looking at the code already...
I'm guessing that also means that fixing it is outside this series' scope.
>>> When vnc_server_info_get() fails, the event is dropped. Why is that
>>> okay? Failure seems unlikely, though.
>>
>> Suggestions what else to do? I don't wanna crash qemu by calling
>> qapi_event_send_vnc_* with a NULL pointer. And, yes, it should be
>> highly unlikely so trying some more sophisticated error handling would
>> probably be dead code ...
>
> These events signal a state change. Dropping them make me feel uneasy,
> because if a client uses them to track state, it gets out of sync.
Events are already best-effort; clients have to be prepared to miss an
event - but that's mainly when reconnecting (such as across libvirtd
restarts), and not while the monitor is reliably connected.
> The events need to identify the server to be of any use for state
> tracking. Currently, this is members host, service, family of data
> member server.
>
> We could avoid failures in vnc_qmp_event() as follows:
>
> 1. When we create a server, we obtain its info with getsockname() and
> getnameinfo(). If they fail, we fail server creation. Else, we
> store the info for vnc_qmp_event().
>
> 2. When a client connects, we obtain its info with getpeername() and
> getnameinfo(). If they fail, we refuse the connection. Else, we
> store the infor for vnc_qmp_event().
Seems reasonable to me, but starts to be out of scope for what I'm
currently tackling, so is this something I can hand to you, Gerd?
>
> Alternatively, we can embrace the failures and send the events without
> the information we failed to get. Requires another way to identify the
> server. VncDisplay's id, perhaps? Might be nice to have anyway.
Yes, including the id as a new field to the event sounds like a
reasonable addition, whether or not my next patch reworking things to
drop info->base makes it impossible to pass qmp_event_send_vnc_* a NULL
pointer, even if we couldn't resolve useful information to display.
--
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 <=
- 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, 2015/10/20
[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