Re: [PATCH v2 04/12] qio: Remember context of qio_net_listener_set_client_func_full

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

On Sat, Nov 08, 2025 at 04:59:25PM -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
> 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 an async 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.
> 
> CC: qemu-stable@nongnu.org
> Fixes: 938c8b79 ("qio: store gsources for net listeners", v2.12.0)
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: move earlier in series, trace context
> ---
>  include/io/net-listener.h |  1 +
>  io/net-listener.c         | 25 ++++++++++++++++---------
>  io/trace-events           |  6 +++---
>  3 files changed, 20 insertions(+), 12 deletions(-)

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 :|