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 <[email protected]>
> ---
> 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é <[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 :|
