[PATCH v2 07/12] qio: Hoist ref of listener outside loop

Eric Blake posted 12 patches 1 week ago
Maintainers: Eric Blake <eblake@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
[PATCH v2 07/12] qio: Hoist ref of listener outside loop
Posted by Eric Blake 1 week ago
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, others 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 the
reference count on the listener will not drop to zero while there are
any pending GSource (since each GSource will not call the notify of
object_unref() until it is no longer servicing a callback); however,
it is also possible to guarantee the same setup by merely holding an
overall reference to the listener as long as there is at least one
GSource, as well as also holding a local reference around any callback
function that has not yet run to completion.

Hoisting the reference like this will make it easier for an upcoming
patch to still ensure the listener cannot be prematurely finalized
during the user's callback, even when using AioContext (where we have
not plumbed in support for a notify function) rather than GSource.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v2: also grab reference around callback execution
---
 io/net-listener.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/io/net-listener.c b/io/net-listener.c
index ad097175eba..dd3522c9b3c 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -54,7 +54,9 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
     trace_qio_net_listener_callback(listener, listener->io_func,
                                     listener->context);
     if (listener->io_func) {
+        object_ref(OBJECT(listener));
         listener->io_func(listener, sioc, listener->io_data);
+        object_unref(OBJECT(listener));
     }

     object_unref(OBJECT(sioc));
@@ -120,12 +122,14 @@ qio_net_listener_watch(QIONetListener *listener, size_t i, const char *caller)

     trace_qio_net_listener_watch(listener, listener->io_func,
                                  listener->context, caller);
-    for ( ; i < listener->nsioc; i++) {
+    if (i == 0) {
         object_ref(OBJECT(listener));
+    }
+    for ( ; i < listener->nsioc; i++) {
         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, listener->context);
+            listener, NULL, listener->context);
     }
 }

@@ -147,6 +151,7 @@ qio_net_listener_unwatch(QIONetListener *listener, const char *caller)
             listener->io_source[i] = NULL;
         }
     }
+    object_unref(OBJECT(listener));
 }

 void qio_net_listener_add(QIONetListener *listener,
-- 
2.51.1
Re: [PATCH v2 07/12] qio: Hoist ref of listener outside loop
Posted by Daniel P. Berrangé 4 days, 10 hours ago
On Sat, Nov 08, 2025 at 04:59:28PM -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, others 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 the
> reference count on the listener will not drop to zero while there are
> any pending GSource (since each GSource will not call the notify of
> object_unref() until it is no longer servicing a callback); however,
> it is also possible to guarantee the same setup by merely holding an
> overall reference to the listener as long as there is at least one
> GSource, as well as also holding a local reference around any callback
> function that has not yet run to completion.
> 
> Hoisting the reference like this will make it easier for an upcoming
> patch to still ensure the listener cannot be prematurely finalized
> during the user's callback, even when using AioContext (where we have
> not plumbed in support for a notify function) rather than GSource.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: also grab reference around callback execution

Still not comfortable with this. I've sent a reply to your v1
patch though, so we keep the analysis in one thread.

> ---
>  io/net-listener.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/io/net-listener.c b/io/net-listener.c
> index ad097175eba..dd3522c9b3c 100644
> --- a/io/net-listener.c
> +++ b/io/net-listener.c
> @@ -54,7 +54,9 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
>      trace_qio_net_listener_callback(listener, listener->io_func,
>                                      listener->context);
>      if (listener->io_func) {
> +        object_ref(OBJECT(listener));
>          listener->io_func(listener, sioc, listener->io_data);
> +        object_unref(OBJECT(listener));
>      }
> 
>      object_unref(OBJECT(sioc));
> @@ -120,12 +122,14 @@ qio_net_listener_watch(QIONetListener *listener, size_t i, const char *caller)
> 
>      trace_qio_net_listener_watch(listener, listener->io_func,
>                                   listener->context, caller);
> -    for ( ; i < listener->nsioc; i++) {
> +    if (i == 0) {
>          object_ref(OBJECT(listener));
> +    }
> +    for ( ; i < listener->nsioc; i++) {
>          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, listener->context);
> +            listener, NULL, listener->context);
>      }
>  }
> 
> @@ -147,6 +151,7 @@ qio_net_listener_unwatch(QIONetListener *listener, const char *caller)
>              listener->io_source[i] = NULL;
>          }
>      }
> +    object_unref(OBJECT(listener));
>  }
> 
>  void qio_net_listener_add(QIONetListener *listener,
> -- 
> 2.51.1
> 

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