On Tue, Oct 20, 2015 at 04:53:24PM -0600, Eric Blake wrote: > 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?
My pending IO channel patches do exactly this Take a look at the QIOChannelSocket impl https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg03440.html This caches the results of getpeername & getsockname in the QOIChannelSocket object. So my patches which convert VNC to use QIOChannelSOcket should solve this particular failure scenario you're discussing. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
