[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: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers |
Date: |
Wed, 21 Oct 2015 12:16:20 +0100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <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.
>
> >>> 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 :|
- [Qemu-devel] [PATCH v9 00/17] qapi collision reduction (post-introspection subset B'), Eric Blake, 2015/10/16
- [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Eric Blake, 2015/10/16
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Gerd Hoffmann, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Eric Blake, 2015/10/20
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/21
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers,
Daniel P. Berrange <=
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Markus Armbruster, 2015/10/23
- Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers, Eric Blake, 2015/10/20
[Qemu-devel] [PATCH v9 10/17] nbd: Convert to new qapi union layout, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 01/17] qapi: Add tests for reserved name abuse, Eric Blake, 2015/10/16
[Qemu-devel] [PATCH v9 08/17] tests: Convert to new qapi union layout, Eric Blake, 2015/10/16