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

[Markus Armbruster <armbru@redhat.com>]

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

>> Outside this patch's scope, but since I'm looking at the code already...
>> 
>> Here's the only caller of vnc_server_info_get():
>> 
>>     static void vnc_qmp_event(VncState *vs, QAPIEvent event)
>>     {
>>         VncServerInfo *si;
>> 
>>         if (!vs->info) {
>>             return;
>>         }
>>         g_assert(vs->info->base);
>> 
>>         si = vnc_server_info_get(vs->vd);
>>         if (!si) {
>> --->        return;
>>         }
>> 
>>         switch (event) {
>>         case QAPI_EVENT_VNC_CONNECTED:
>>             qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
>>             break;
>>         case QAPI_EVENT_VNC_INITIALIZED:
>>             qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
>>             break;
>>         case QAPI_EVENT_VNC_DISCONNECTED:
>>             qapi_event_send_vnc_disconnected(si, vs->info, &error_abort);
>>             break;
>>         default:
>>             break;
>>         }
>> 
>>         qapi_free_VncServerInfo(si);
>>     }
>> 
>> 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.

What's the state machine here?  If I understand things correctly, we
have a number of VNC servers, tracked in vnc_displays, which is a list
of VncDisplay.  Each of them goes through the following states:

* Unconnected
  On connect, emit VNC_CONNECTED and go to Pre-auth

* Pre-auth
  On authentication, emit VNC_INITIALIZED, and go to Active
  On disconnect, emit VNC_DISCONNECTED, and go to Unconnected

* Active
  On disconnect, emit VNC_DISCONNECTED, and go to Unconnected

Correct?

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

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.