Re: [Qemu-devel] [PATCH v9 04/17] vnc: hoist allocation of VncBasicInfo to callers

[Markus Armbruster <armbru@redhat.com>]

"Daniel P. Berrange" <berrange@redhat.com> writes:

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

Great!  No need to add a FIXME comment then.