Re: [PATCH 3/8] qio: Remember context of qio_net_listener_set_client_func_full

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Nov 03, 2025 at 02:10:54PM -0600, Eric Blake wrote:
> io/net-listener.c has two modes of use: asynchronous (the user calls
> qio_net_listener_set_client_func to wake up the callback via the
> global GMainContext, or qio_net_listener_set_client_func_full to wake
> up the callback via the caller's own alternative GMainContext), and
> synchronous (the user calls qio_net_listener_wait_client which creates
> its own GMainContext and waits for the first client connection before
> returning, with no need for a user's callback).  But commit 938c8b79
> ("qio: store gsources for net listeners", v2.12.0) has a latent logic
> flaw: when qio_net_listener_wait_client finishes on its temporary
> context, it reverts all of the siocs back to the global GMainContext
> rather than the potentially non-NULL context they might have been
> originally registered with.  Similarly, if the user creates a
> net-listener, adds initial addresses, registers an async callback with
> a non-default context (which ties to all siocs for the initial
> addresses), then adds more addresses with qio_net_listener_add, the
> siocs for later addresses are blindly placed in the global context,
> rather than sharing the context of the earlier ones.
> 
> In practice, I don't think this has caused issues.  As pointed out by
> the original commit, all async callers prior to that commit were
> already okay with the NULL default context; and the typical usage
> pattern is to first add ALL the addresses the listener will pay
> attention to before ever setting the async callback.  Likewise, if a
> file uses only qio_net_listener_set_client_func instead of
> qio_net_listener_set_client_func_full, then it is never using a custom
> context, so later assignments of async callbacks will still be to the
> same global context as earlier ones.  Meanwhile, any callers that want
> to do the sync operation to grab the first client are unlikely to
> register an async callback; altogether bypassing the question of
> whether later assignments of a GSource are being tied to a different
> context over time.
> 
> I do note that chardev/char-socket.c is the only file that calls both
> qio_net_listener_wait_client (sync for a single client in
> tcp_chr_accept_server_sync), and qio_net_listener_set_client_func_full
> (several places, all with chr->gcontext, but sometimes with a NULL
> callback function during teardown).  But as far as I can tell, the two
> uses are mutually exclusive, based on the is_waitconnect parameter to
> qmp_chardev_open_socket_server.
> 
> That said, it is more robust to remember when a callback function is
> tied to a non-default context, and have both the sync wait and any
> late address additions honor that same context.  That way, the code
> will be robust even if a later user performs a sync wait for a
> specific client in the middle of servicing a longer-lived
> QIONetListener that has an async callback for all other clients.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/io/net-listener.h | 1 +
>  io/net-listener.c         | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> index ab9f291ed62..42fbfab5467 100644
> --- a/include/io/net-listener.h
> +++ b/include/io/net-listener.h
> @@ -50,6 +50,7 @@ struct QIONetListener {
>      QIOChannelSocket **sioc;
>      GSource **io_source;
>      size_t nsioc;
> +    GMainContext *context;
> 
>      bool connected;
> 
> diff --git a/io/net-listener.c b/io/net-listener.c
> index e89286ea63c..15df673fb6e 100644
> --- a/io/net-listener.c
> +++ b/io/net-listener.c
> @@ -132,7 +132,7 @@ void qio_net_listener_add(QIONetListener *listener,
>          listener->io_source[listener->nsioc] = qio_channel_add_watch_source(
>              QIO_CHANNEL(listener->sioc[listener->nsioc]), G_IO_IN,
>              qio_net_listener_channel_func,
> -            listener, (GDestroyNotify)object_unref, NULL);
> +            listener, (GDestroyNotify)object_unref, listener->context);
>      }
> 
>      listener->nsioc++;
> @@ -160,6 +160,7 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
>      listener->io_func = func;
>      listener->io_data = data;
>      listener->io_notify = notify;
> +    listener->context = context;


The previous patch added a short circuit condition:

    if (listener->io_func == func && listener->io_data == data) {
        return;
    }

I feel like we should have  "&& listener->contxt == context" in
the short circuit check too


> 
>      for (i = 0; i < listener->nsioc; i++) {
>          if (listener->io_source[i]) {
> @@ -271,7 +272,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
>              listener->io_source[i] = qio_channel_add_watch_source(
>                  QIO_CHANNEL(listener->sioc[i]), G_IO_IN,
>                  qio_net_listener_channel_func,
> -                listener, (GDestroyNotify)object_unref, NULL);
> +                listener, (GDestroyNotify)object_unref, listener->context);
>          }
>      }

If we extend the above short circuit condition then...

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|