From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49164) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZorNj-00040n-Rg for qemu-devel@nongnu.org; Wed, 21 Oct 2015 07:16:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZorNY-0004m1-Qw for qemu-devel@nongnu.org; Wed, 21 Oct 2015 07:16:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60161) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZorNY-0004lw-IF for qemu-devel@nongnu.org; Wed, 21 Oct 2015 07:16:24 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 0C4622EA for ; Wed, 21 Oct 2015 11:16:24 +0000 (UTC) Date: Wed, 21 Oct 2015 12:16:20 +0100 From: "Daniel P. Berrange" Message-ID: <20151021111620.GC21888@redhat.com> References: <1444968943-11254-1-git-send-email-eblake@redhat.com> <1444968943-11254-5-git-send-email-eblake@redhat.com> <87h9lmat1b.fsf@blackfin.pond.sub.org> <1445331261.13733.65.camel@redhat.com> <87io614myv.fsf@blackfin.pond.sub.org> <5626C5E4.1010308@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5626C5E4.1010308@redhat.com> Subject: Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Markus Armbruster , Gerd Hoffmann 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 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 :|