[PATCH v3 09/13] qio: Provide accessor around QIONetListener->sioc

[PATCH v3 09/13] qio: Provide accessor around QIONetListener->sioc

[Eric Blake]

An upcoming patch needs to pass more than just sioc as the opaque
pointer to an AioContext; but since our AioContext code in general
(and its QIO Channel wrapper code) lacks a notify callback present
with GSource, we do not have the trivial option of just g_malloc'ing a
small struct to hold all that data coupled with a notify of g_free.
Instead, the data pointer must outlive the registered handler; in
fact, having the data pointer have the same lifetime as QIONetListener
is adequate.

But the cleanest way to stick such a helper struct in QIONetListener
will be to rearrange internal struct members.  And that in turn means
that all existing code that currently directly accesses
listener->nsioc and listener->sioc[] should instead go through
accessor functions, to be immune to the upcoming struct layout
changes.  So this patch adds accessor methods qio_net_listener_nsioc()
and qio_net_listener_sioc(), and puts them to use.

While at it, notice that the pattern of grabbing an sioc from the
listener only to turn around can call
qio_channel_socket_get_local_address is common enough to also warrant
the helper of qio_net_listener_get_local_address, and fix a copy-paste
error in the corresponding documentation.

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

---
v2: new patch
v3: fix off-by-one, also add qio_net_listener_get_local_address
---
 include/io/channel-socket.h |  2 +-
 include/io/net-listener.h   | 42 +++++++++++++++++++++++++++++++++++++
 chardev/char-socket.c       |  2 +-
 io/net-listener.c           | 27 ++++++++++++++++++++++++
 migration/socket.c          |  4 ++--
 ui/vnc.c                    | 34 ++++++++++++++++++------------
 6 files changed, 94 insertions(+), 17 deletions(-)

diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index fcfd489c6ce..a1ef3136ea2 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -228,7 +228,7 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
  * release with a call qapi_free_SocketAddress() when no
  * longer required.
  *
- * Returns: 0 on success, -1 on error
+ * Returns: the socket address struct, or NULL on error
  */
 SocketAddress *
 qio_channel_socket_get_local_address(QIOChannelSocket *ioc,
diff --git a/include/io/net-listener.h b/include/io/net-listener.h
index c2165dc1669..95bc7407d66 100644
--- a/include/io/net-listener.h
+++ b/include/io/net-listener.h
@@ -185,4 +185,46 @@ void qio_net_listener_disconnect(QIONetListener *listener);
  */
 bool qio_net_listener_is_connected(QIONetListener *listener);

+
+/**
+ * qio_net_listener_nsioc:
+ * @listener: the network listener object
+ *
+ * Determine the number of listener channels currently owned by the
+ * given listener.
+ *
+ * Returns: number of channels, or 0 if not listening
+ */
+size_t qio_net_listener_nsioc(QIONetListener *listener);
+
+
+/**
+ * qio_net_listener_sioc:
+ * @listener: the network listener object
+ * @n: index of the sioc to grab
+ *
+ * Accessor for the nth sioc owned by the listener.
+ *
+ * Returns: the requested listener, or NULL if not in bounds
+ */
+QIOChannelSocket *qio_net_listener_sioc(QIONetListener *listener, size_t n);
+
+/**
+ * qio_net_listener_get_local_address:
+ * @listener: the network listener object
+ * @n: index of the sioc to grab
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Get the string representation of the local socket
+ * address. A pointer to the allocated address information
+ * struct will be returned, which the caller is required to
+ * release with a call qapi_free_SocketAddress() when no
+ * longer required.
+ *
+ * Returns: the socket address struct, or NULL on error
+ */
+SocketAddress *
+qio_net_listener_get_local_address(QIONetListener *listener, size_t n,
+                                   Error **errp);
+
 #endif /* QIO_NET_LISTENER_H */
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index ec4116ade4d..26d2f11202e 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1255,7 +1255,7 @@ static int qmp_chardev_open_socket_server(Chardev *chr,
     }

     qapi_free_SocketAddress(s->addr);
-    s->addr = qio_channel_socket_get_local_address(s->listener->sioc[0], errp);
+    s->addr = qio_net_listener_get_local_address(s->listener, 0, errp);

 skip_listen:
     update_disconnected_filename(s);
diff --git a/io/net-listener.c b/io/net-listener.c
index 9a94b153279..9ffbc141a76 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -316,6 +316,33 @@ bool qio_net_listener_is_connected(QIONetListener *listener)
     return listener->connected;
 }

+size_t qio_net_listener_nsioc(QIONetListener *listener)
+{
+    return listener->nsioc;
+}
+
+QIOChannelSocket *qio_net_listener_sioc(QIONetListener *listener, size_t n)
+{
+    if (n >= listener->nsioc) {
+        return NULL;
+    }
+    return listener->sioc[n];
+}
+
+SocketAddress *
+qio_net_listener_get_local_address(QIONetListener *listener, size_t n,
+                                   Error **errp)
+{
+    QIOChannelSocket *sioc = qio_net_listener_sioc(listener, n);
+
+    if (!sioc) {
+        error_setg(errp, "Listener index out of range");
+        return NULL;
+    }
+
+    return qio_channel_socket_get_local_address(sioc, errp);
+}
+
 static void qio_net_listener_finalize(Object *obj)
 {
     QIONetListener *listener = QIO_NET_LISTENER(obj);
diff --git a/migration/socket.c b/migration/socket.c
index 5ec65b8c039..9e379bf56fd 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -170,9 +170,9 @@ void socket_start_incoming_migration(SocketAddress *saddr,
                                           NULL, NULL,
                                           g_main_context_get_thread_default());

-    for (i = 0; i < listener->nsioc; i++)  {
+    for (i = 0; i < qio_net_listener_nsioc(listener); i++)  {
         SocketAddress *address =
-            qio_channel_socket_get_local_address(listener->sioc[i], errp);
+            qio_net_listener_get_local_address(listener, i, errp);
         if (!address) {
             return;
         }
diff --git a/ui/vnc.c b/ui/vnc.c
index 50016ff7ab4..0d499b208b9 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -235,12 +235,12 @@ static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
     VncServerInfo *info;
     Error *err = NULL;

-    if (!vd->listener || !vd->listener->nsioc) {
+    if (!vd->listener || !qio_net_listener_nsioc(vd->listener)) {
         return NULL;
     }

     info = g_malloc0(sizeof(*info));
-    vnc_init_basic_info_from_server_addr(vd->listener->sioc[0],
+    vnc_init_basic_info_from_server_addr(qio_net_listener_sioc(vd->listener, 0),
                                          qapi_VncServerInfo_base(info), &err);
     info->auth = g_strdup(vnc_auth_name(vd));
     if (err) {
@@ -377,7 +377,7 @@ VncInfo *qmp_query_vnc(Error **errp)
     VncDisplay *vd = vnc_display_find(NULL);
     SocketAddress *addr = NULL;

-    if (vd == NULL || !vd->listener || !vd->listener->nsioc) {
+    if (vd == NULL || !vd->listener || !qio_net_listener_nsioc(vd->listener)) {
         info->enabled = false;
     } else {
         info->enabled = true;
@@ -386,8 +386,7 @@ VncInfo *qmp_query_vnc(Error **errp)
         info->has_clients = true;
         info->clients = qmp_query_client_list(vd);

-        addr = qio_channel_socket_get_local_address(vd->listener->sioc[0],
-                                                    errp);
+        addr = qio_net_listener_get_local_address(vd->listener, 0, errp);
         if (!addr) {
             goto out_error;
         }
@@ -549,6 +548,8 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
     size_t i;

     QTAILQ_FOREACH(vd, &vnc_displays, next) {
+        size_t nsioc = 0;
+
         info = g_new0(VncInfo2, 1);
         info->id = g_strdup(vd->id);
         info->clients = qmp_query_client_list(vd);
@@ -559,14 +560,21 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
                                                   "device", &error_abort));
             info->display = g_strdup(dev->id);
         }
-        for (i = 0; vd->listener != NULL && i < vd->listener->nsioc; i++) {
-            info->server = qmp_query_server_entry(
-                vd->listener->sioc[i], false, vd->auth, vd->subauth,
-                info->server);
+        if (vd->listener != NULL) {
+            nsioc = qio_net_listener_nsioc(vd->listener);
         }
-        for (i = 0; vd->wslistener != NULL && i < vd->wslistener->nsioc; i++) {
+        for (i = 0; i < nsioc; i++) {
             info->server = qmp_query_server_entry(
-                vd->wslistener->sioc[i], true, vd->ws_auth,
+                qio_net_listener_sioc(vd->listener, i), false, vd->auth,
+                vd->subauth, info->server);
+        }
+        nsioc = 0;
+        if (vd->wslistener) {
+            nsioc = qio_net_listener_nsioc(vd->wslistener);
+        }
+        for (i = 0; i < nsioc; i++) {
+            info->server = qmp_query_server_entry(
+                qio_net_listener_sioc(vd->wslistener, i), true, vd->ws_auth,
                 vd->ws_subauth, info->server);
         }

@@ -3550,11 +3558,11 @@ static void vnc_display_print_local_addr(VncDisplay *vd)
 {
     SocketAddress *addr;

-    if (!vd->listener || !vd->listener->nsioc) {
+    if (!vd->listener || !qio_net_listener_nsioc(vd->listener)) {
         return;
     }

-    addr = qio_channel_socket_get_local_address(vd->listener->sioc[0], NULL);
+    addr = qio_net_listener_get_local_address(vd->listener, 0, NULL);
     if (!addr) {
         return;
     }
-- 
2.51.1

Re: [PATCH v3 05/13] qio: Protect NetListener callback with mutex

[Daniel P. Berrangé]

On Wed, Nov 12, 2025 at 04:31:05PM -0600, Eric Blake wrote:
> Without a mutex, NetListener can run into this data race between a
> thread changing the async callback callback function to use when a
> client connects, and the thread servicing polling of the listening
> sockets:
> 
>  Thread 1:
>        qio_net_listener_set_client_func(lstnr, f1, ...);
>            => foreach sock: socket
>                => object_ref(lstnr)
>                => sock_src = qio_channel_socket_add_watch_source(sock, ...., lstnr, object_unref);
> 
>   Thread 2:
>        poll()
>           => event POLLIN on socket
>                => ref(GSourceCallback)
>                => if (lstnr->io_func) // while lstnr->io_func is f1
>                     ...interrupt..
> 
>   Thread 1:
>        qio_net_listener_set_client_func(lstnr, f2, ...);
>           => foreach sock: socket
>                => g_source_unref(sock_src)
>           => foreach sock: socket
>                => object_ref(lstnr)
>                => sock_src = qio_channel_socket_add_watch_source(sock, ...., lstnr, object_unref);
> 
>   Thread 2:
>                => call lstnr->io_func(lstnr->io_data) // now sees f2
>                => return dispatch(sock)
>                => unref(GSourceCallback)
>                   => destroy-notify
>                      => object_unref
> 
> Found by inspection.  This is a SEGFAULT waiting to happen if f2 can
> become NULL because thread 1 deregisters the user's callback while
> thread 2 is trying to service the callback.  Other messes are also
> theoretically possible, such as running callback f1 with an opaque
> pointer that should only be passed to f2 (if the client code were to
> use more than just a binary choice between a single async function or
> NULL).
> 
> Mitigating factor: if the code that modifies the QIONetListener can
> only be reached by the same thread that is executing the polling and
> async callbacks, then we are not in a two-thread race documented above
> (even though poll can see two clients trying to connect in the same
> window of time, any changes made to the listener by the first async
> callback will be completed before the thread moves on to the second
> client).  However, QEMU is complex enough that I was unable to state
> with certainty whether a QMP command (such as nbd-server-stop, which
> does modify the net listener) can ever be serviced in a thread
> distinct from the one trying to do the async callbacks.  Similarly, I
> did not spend the time trying to add sleeps or execute under gdb to
> try and actually trigger the race in practice.
> 
> At any rate, it is worth having the API be robust.  To ensure that
> modifying a NetListener can be safely done from any thread, add a
> mutex that guarantees atomicity to all members of a listener object
> related to callbacks.  This problem has been present since
> QIONetListener was introduced.
> 
> Note that this does NOT prevent the case of a second round of the
> user's old async callback being invoked with the old opaque data, even
> when the user has already tried to change the async callback during
> the first async callback; it is only about ensuring that there is no
> sharding (the eventual io_func(io_data) call that does get made will
> correspond to a particular combination that the user had requested at
> some point in time, and not be sharded to a combination that never
> existed in practice).  In other words, this patch maintains the status
> quo that a user's async callback function already needs to be robust
> to parallel clients landing in the same window of poll servicing, even
> when only one client is desired.
> 
> CC: qemu-stable@nongnu.org
> Fixes: 53047392 ("io: introduce a network socket listener API", v2.12.0)
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: new patch
> ---
>  include/io/net-listener.h |  1 +
>  io/net-listener.c         | 58 +++++++++++++++++++++++++++++----------
>  2 files changed, 44 insertions(+), 15 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 :|

Re: [PATCH v3 07/13] qio: Factor out helpers qio_net_listener_[un]watch

[Daniel P. Berrangé]

On Wed, Nov 12, 2025 at 04:31:07PM -0600, Eric Blake wrote:
> The code had three similar repetitions of an iteration over one or all
> of nsiocs to set up a GSource, and likewise for teardown.  Since an
> upcoming patch wants to tweak whether GSource or AioContext is used,
> its better to consolidate that into one helper function for fewer
> places to edit later.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: rebase to changes on the tracepoints earlier in series
> v3: rebase to mutex protections, R-b dropped
> ---
>  io/net-listener.c | 122 ++++++++++++++++++++--------------------------
>  1 file changed, 52 insertions(+), 70 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 :|

Re: [PATCH v3 08/13] chardev: Reuse channel's cached local address

[Daniel P. Berrangé]

On Wed, Nov 12, 2025 at 04:31:08PM -0600, Eric Blake wrote:
> Directly accessing the fd member of a QIOChannelSocket is an
> undesirable leaky abstraction.  What's more, grabbing that fd merely
> to force an eventual call to getsockname() can be wasteful, since the
> channel is often able to return its cached local name.
> 
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: new patch
> ---
>  chardev/char-socket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH v3 09/13] qio: Provide accessor around QIONetListener->sioc

[Daniel P. Berrangé]

On Wed, Nov 12, 2025 at 04:31:09PM -0600, Eric Blake wrote:
> An upcoming patch needs to pass more than just sioc as the opaque
> pointer to an AioContext; but since our AioContext code in general
> (and its QIO Channel wrapper code) lacks a notify callback present
> with GSource, we do not have the trivial option of just g_malloc'ing a
> small struct to hold all that data coupled with a notify of g_free.
> Instead, the data pointer must outlive the registered handler; in
> fact, having the data pointer have the same lifetime as QIONetListener
> is adequate.
> 
> But the cleanest way to stick such a helper struct in QIONetListener
> will be to rearrange internal struct members.  And that in turn means
> that all existing code that currently directly accesses
> listener->nsioc and listener->sioc[] should instead go through
> accessor functions, to be immune to the upcoming struct layout
> changes.  So this patch adds accessor methods qio_net_listener_nsioc()
> and qio_net_listener_sioc(), and puts them to use.
> 
> While at it, notice that the pattern of grabbing an sioc from the
> listener only to turn around can call
> qio_channel_socket_get_local_address is common enough to also warrant
> the helper of qio_net_listener_get_local_address, and fix a copy-paste
> error in the corresponding documentation.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: new patch
> v3: fix off-by-one, also add qio_net_listener_get_local_address
> ---
>  include/io/channel-socket.h |  2 +-
>  include/io/net-listener.h   | 42 +++++++++++++++++++++++++++++++++++++
>  chardev/char-socket.c       |  2 +-
>  io/net-listener.c           | 27 ++++++++++++++++++++++++
>  migration/socket.c          |  4 ++--
>  ui/vnc.c                    | 34 ++++++++++++++++++------------
>  6 files changed, 94 insertions(+), 17 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 :|

Re: [PATCH v3 11/13] qio: Add QIONetListener API for using AioContext

[Daniel P. Berrangé]

On Wed, Nov 12, 2025 at 04:31:11PM -0600, Eric Blake wrote:
> The user calling himself "John Doe" reported a deadlock when
> attempting to use qemu-storage-daemon to serve both a base file over
> NBD, and a qcow2 file with that NBD export as its backing file, from
> the same process, even though it worked just fine when there were two
> q-s-d processes.  The bulk of the NBD server code properly uses
> coroutines to make progress in an event-driven manner, but the code
> for spawning a new coroutine at the point when listen(2) detects a new
> client was hard-coded to use the global GMainContext; in other words,
> the callback that triggers nbd_client_new to let the server start the
> negotiation sequence with the client requires the main loop to be
> making progress.  However, the code for bdrv_open of a qcow2 image
> with an NBD backing file uses an AIO_WAIT_WHILE nested event loop to
> ensure that the entire qcow2 backing chain is either fully loaded or
> rejected, without any side effects from the main loop causing unwanted
> changes to the disk being loaded (in short, an AioContext represents
> the set of actions that are known to be safe while handling block
> layer I/O, while excluding any other pending actions in the global
> main loop with potentially larger risk of unwanted side effects).
> 
> This creates a classic case of deadlock: the server can't progress to
> the point of accept(2)ing the client to write to the NBD socket
> because the main loop is being starved until the AIO_WAIT_WHILE
> completes the bdrv_open, but the AIO_WAIT_WHILE can't progress because
> it is blocked on the client coroutine stuck in a read() of the
> expected magic number from the server side of the socket.
> 
> This patch adds a new API to allow clients to opt in to listening via
> an AioContext rather than a GMainContext.  This will allow NBD to fix
> the deadlock by performing all actions during bdrv_open in the main
> loop AioContext.
> 
> Technical debt warning: I would have loved to utilize a notify
> function with AioContext to guarantee that we don't finalize listener
> due to an object_unref if there is any callback still running (the way
> GSource does), but wiring up notify functions into AioContext is a
> bigger task that will be deferred to a later QEMU release.  But for
> solving the NBD deadlock, it is sufficient to note that the QMP
> commands for enabling and disabling the NBD server are really the only
> points where we want to change the listener's callback.  Furthermore,
> those commands are serviced in the main loop, which is the same
> AioContext that is also listening for connections.  Since a thread
> cannot interrupt itself, we are ensured that at the point where we are
> changing the watch, there are no callbacks active.  This is NOT as
> powerful as the GSource cross-thread safety, but sufficient for the
> needs of today.
> 
> An upcoming patch will then add a unit test (kept separate to make it
> easier to rearrange the series to demonstrate the deadlock without
> this patch).
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3169
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: Retitle and add new API rather than changing semantics of
> existing qio_net_listener_set_client_func; use qio accessor rather
> than direct access to the sioc fd and a lower-level aio call
> v3: limit reference counting change to just AioContext, R-b dropped
> ---
>  include/io/net-listener.h | 21 +++++++++++++
>  io/net-listener.c         | 66 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 84 insertions(+), 3 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 :|