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: [email protected] > Fixes: 938c8b79 ("qio: store gsources for net listeners", v2.12.0) > Signed-off-by: Eric Blake <[email protected]> > > --- > 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é <[email protected]> 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 :|
