[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers |
Date: |
Wed, 21 Oct 2015 13:02:56 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> 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.
Definitely.
>>>> 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.
Big difference, actually!
Scenario A: events are reliable while you're connected, but you may miss
some during a reconnect. You have to poll the state on connect to
compensate for missed events.
Scenatio B: events are a best effort. You have to poll the state
periodically to compensate for dropped events.
In my understanding, we (mean to) provide A.
>> 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.
I don't think the solution to this problem will depend on your QAPI
work.
- [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 <=
- 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