qemu-devel
[Top][All Lists]
Advanced

[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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]