On Tue, Nov 04, 2025 at 11:13:48AM +0000, Daniel P. Berrangé wrote: > On Mon, Nov 03, 2025 at 02:10:57PM -0600, Eric Blake wrote: > > The point of QIONetListener is to allow a server to listen to more > > than one socket address at a time, and respond to clients in a > > first-come-first-serve order across any of those addresses. While > > some servers (like NBD) really do want to serve multiple simultaneous > > clients, many other servers only care about the first client to > > connect, and will immediately deregister the callback, possibly by > > dropping their reference to the QIONetListener. The existing code > > ensures that all other pending callbacks remain safe once the first > > callback drops the listener, by adding an extra reference to the > > listener for each GSource created, where those references pair to the > > eventual teardown of each GSource after a given callbacks has been > > serviced or aborted. But it is equally acceptable to hoist the > > reference to the listener outside the loop - as long as there is a > > callback function registered, it is sufficient to have a single > > reference live for the entire array of sioc, rather than one reference > > per sioc in the array. > > > > Hoisting the reference like this will make it easier for an upcoming > > patch to still ensure the listener cannot be prematurely garbage > > collected during the user's callback, even when the callback no longer > > uses a per-sioc GSource. > > It isn't quite this simple. Glib reference counts the callback > func / data, holding a reference when dispatching the callback. > > IOW, even if the GSource is unrefed, the callback 'notify' > function won't be called if the main loop is in the process > of dispatching.
I'm not sure I follow your argument. Glib holds a reference on the GSource object, not on the opaque data that is handed to the GSource. It is possible to use g_source_set_callback_indirect() where GSource can learn how to use the same reference counting on data as external code, by the use of function pointers for ref and unref, but QIO uses merely g_source_set_callback(). https://gitlab.gnome.org/GNOME/glib/-/blob/main/glib/gmain.c#L1844 shows that glib then wraps that opaque pointer into an internal GSourceCallback object which itself is reference counted, so that the notify function is not called until the GSource is finalized, but that is reference counting on the container, not on the opaque object itself (which in this patch is the QIONetListener). > > With this change, the reference on 'listener' can now be > released even if the callback is currently dispatching. So if I'm understanding your concern, you're worried that the unwatch code can finish looping through the g_source_destroy and then reach the point where it unrefs listener, but that a late-breaking client connection can trigger a callback that can still be executing in another thread/coroutine after the listener is unref'ed but before the GSource has been finalized? If so, would squashing this in fix the problem you are seeing? diff --git i/io/net-listener.c w/io/net-listener.c index 9f4e3c0be0c..1fcbbeb7a76 100644 --- i/io/net-listener.c +++ w/io/net-listener.c @@ -67,8 +67,10 @@ static void qio_net_listener_aio_func(void *opaque) { QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(opaque); + object_ref(OBJECT(sioc->listener)); qio_net_listener_channel_func(QIO_CHANNEL(sioc), G_IO_IN, sioc->listener); + object_unref(OBJECT(sioc->listener)); } -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
