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