Eric Blake <ebl...@redhat.com> writes: > On 10/20/2015 08:46 AM, Markus Armbruster wrote: >> Gerd Hoffmann <kra...@redhat.com> 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.
