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>
---
io/net-listener.c | 109 +++++++++++++++++++---------------------------
1 file changed, 45 insertions(+), 64 deletions(-)
diff --git a/io/net-listener.c b/io/net-listener.c
index 15df673fb6e..e1378b9a612 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -106,6 +106,45 @@ int qio_net_listener_open_sync(QIONetListener *listener,
}
}
+/*
+ * i == 0 to set watch on entire array, non-zero to only set watch on
+ * recent additions when earlier entries are already watched.
+ */
+static void
+qio_net_listener_watch(QIONetListener *listener, size_t i, const char *caller)
+{
+ if (!listener->io_func) {
+ return;
+ }
+
+ trace_qio_net_listener_watch_enabled(listener, listener->io_func, caller);
+ for ( ; i < listener->nsioc; i++) {
+ object_ref(OBJECT(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, listener->context);
+ }
+}
+
+static void
+qio_net_listener_unwatch(QIONetListener *listener, const char *caller)
+{
+ size_t i;
+
+ if (!listener->io_func) {
+ return;
+ }
+
+ trace_qio_net_listener_watch_disabled(listener, caller);
+ for (i = 0; i < listener->nsioc; i++) {
+ if (listener->io_source[i]) {
+ g_source_destroy(listener->io_source[i]);
+ g_source_unref(listener->io_source[i]);
+ listener->io_source[i] = NULL;
+ }
+ }
+}
void qio_net_listener_add(QIONetListener *listener,
QIOChannelSocket *sioc)
@@ -125,17 +164,7 @@ void qio_net_listener_add(QIONetListener *listener,
object_ref(OBJECT(sioc));
listener->connected = true;
- if (listener->io_func != NULL) {
- trace_qio_net_listener_watch_enabled(listener, listener->io_func,
- "add");
- object_ref(OBJECT(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, listener->context);
- }
-
- listener->nsioc++;
+ qio_net_listener_watch(listener, listener->nsioc++, "add");
}
@@ -145,15 +174,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
GDestroyNotify notify,
GMainContext *context)
{
- size_t i;
-
if (listener->io_func == func && listener->io_data == data) {
return;
}
- if (listener->io_func) {
- trace_qio_net_listener_watch_disabled(listener, "set_client_func");
- }
+ qio_net_listener_unwatch(listener, "set_client_func");
if (listener->io_notify) {
listener->io_notify(listener->io_data);
}
@@ -162,25 +187,7 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
listener->io_notify = notify;
listener->context = context;
- for (i = 0; i < listener->nsioc; i++) {
- if (listener->io_source[i]) {
- g_source_destroy(listener->io_source[i]);
- g_source_unref(listener->io_source[i]);
- listener->io_source[i] = NULL;
- }
- }
-
- if (listener->io_func != NULL) {
- trace_qio_net_listener_watch_enabled(listener, listener->io_func,
- "set_client_func");
- for (i = 0; i < listener->nsioc; i++) {
- object_ref(OBJECT(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, context);
- }
- }
+ qio_net_listener_watch(listener, 0, "set_client_func");
}
void qio_net_listener_set_client_func(QIONetListener *listener,
@@ -232,16 +239,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
};
size_t i;
- if (listener->io_func) {
- trace_qio_net_listener_watch_disabled(listener, "wait_client");
- }
- for (i = 0; i < listener->nsioc; i++) {
- if (listener->io_source[i]) {
- g_source_destroy(listener->io_source[i]);
- g_source_unref(listener->io_source[i]);
- listener->io_source[i] = NULL;
- }
- }
+ qio_net_listener_unwatch(listener, "wait_client");
sources = g_new0(GSource *, listener->nsioc);
for (i = 0; i < listener->nsioc; i++) {
@@ -264,17 +262,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
g_main_loop_unref(loop);
g_main_context_unref(ctxt);
- if (listener->io_func != NULL) {
- trace_qio_net_listener_watch_enabled(listener, listener->io_func,
- "wait_client");
- for (i = 0; i < listener->nsioc; i++) {
- object_ref(OBJECT(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, listener->context);
- }
- }
+ qio_net_listener_watch(listener, 0, "wait_client");
return data.sioc;
}
@@ -287,15 +275,8 @@ void qio_net_listener_disconnect(QIONetListener *listener)
return;
}
- if (listener->io_func) {
- trace_qio_net_listener_watch_disabled(listener, "disconnect");
- }
+ qio_net_listener_unwatch(listener, "disconnect");
for (i = 0; i < listener->nsioc; i++) {
- if (listener->io_source[i]) {
- g_source_destroy(listener->io_source[i]);
- g_source_unref(listener->io_source[i]);
- listener->io_source[i] = NULL;
- }
qio_channel_close(QIO_CHANNEL(listener->sioc[i]), NULL);
}
listener->connected = false;
--
2.51.1Am 03.11.2025 um 21:10 hat Eric Blake geschrieben:
> 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>
> ---
> io/net-listener.c | 109 +++++++++++++++++++---------------------------
> 1 file changed, 45 insertions(+), 64 deletions(-)
> @@ -145,15 +174,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
> GDestroyNotify notify,
> GMainContext *context)
> {
> - size_t i;
> -
> if (listener->io_func == func && listener->io_data == data) {
> return;
> }
>
> - if (listener->io_func) {
> - trace_qio_net_listener_watch_disabled(listener, "set_client_func");
> - }
> + qio_net_listener_unwatch(listener, "set_client_func");
> if (listener->io_notify) {
> listener->io_notify(listener->io_data);
> }
This changes the order between the io_notify() call and the unwatch. Is
this intentional? If so, maybe mention it in the commit message and why
it's safe.
Kevin
On Tue, Nov 04, 2025 at 01:37:34PM +0100, Kevin Wolf wrote:
> Am 03.11.2025 um 21:10 hat Eric Blake geschrieben:
> > 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>
> > ---
> > io/net-listener.c | 109 +++++++++++++++++++---------------------------
> > 1 file changed, 45 insertions(+), 64 deletions(-)
>
> > @@ -145,15 +174,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
> > GDestroyNotify notify,
> > GMainContext *context)
> > {
> > - size_t i;
> > -
> > if (listener->io_func == func && listener->io_data == data) {
> > return;
> > }
> >
> > - if (listener->io_func) {
> > - trace_qio_net_listener_watch_disabled(listener, "set_client_func");
> > - }
> > + qio_net_listener_unwatch(listener, "set_client_func");
> > if (listener->io_notify) {
> > listener->io_notify(listener->io_data);
> > }
>
> This changes the order between the io_notify() call and the unwatch. Is
> this intentional? If so, maybe mention it in the commit message and why
> it's safe.
At least conceptually I think this ordering is better, and I don't think
there should be any functional consequences from the change.
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 :|
On Tue, Nov 04, 2025 at 01:10:12PM +0000, Daniel P. Berrangé wrote:
> On Tue, Nov 04, 2025 at 01:37:34PM +0100, Kevin Wolf wrote:
> > Am 03.11.2025 um 21:10 hat Eric Blake geschrieben:
> > > 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>
> > > ---
> > > io/net-listener.c | 109 +++++++++++++++++++---------------------------
> > > 1 file changed, 45 insertions(+), 64 deletions(-)
> >
> > > @@ -145,15 +174,11 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
> > > GDestroyNotify notify,
> > > GMainContext *context)
> > > {
> > > - size_t i;
> > > -
> > > if (listener->io_func == func && listener->io_data == data) {
> > > return;
> > > }
> > >
> > > - if (listener->io_func) {
> > > - trace_qio_net_listener_watch_disabled(listener, "set_client_func");
> > > - }
> > > + qio_net_listener_unwatch(listener, "set_client_func");
> > > if (listener->io_notify) {
> > > listener->io_notify(listener->io_data);
> > > }
> >
> > This changes the order between the io_notify() call and the unwatch. Is
> > this intentional? If so, maybe mention it in the commit message and why
> > it's safe.
>
> At least conceptually I think this ordering is better, and I don't think
> there should be any functional consequences from the change.
>
It may have been inadvertent at the time I wrote the patch, but I
agree it makes sense to call it out as intentional in v2.
Before this patch series, io_notify is typically object_unref on an
object that has other references besides the one associated with the
NetListener. In which case, calling io_notify() reduces the refcount,
for example from 2 to 1, but has no other impact in relation to
whether the GSource is still armed. More drastic would be if the
io_notify() reduced the refcount from 1 to 0 and thereby finalized the
data. If the GSource is still armed and has any chance at all of a
narrow window of time where it can fire before being unwatched, but
the io_data has been freed because the io_notify() dropped the
refcount to 0 rather than merely reducing it, then the callback can
risk a use-after-free of the io_data.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
On Mon, Nov 03, 2025 at 02:10:55PM -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>
> ---
> io/net-listener.c | 109 +++++++++++++++++++---------------------------
> 1 file changed, 45 insertions(+), 64 deletions(-)
>
> diff --git a/io/net-listener.c b/io/net-listener.c
> index 15df673fb6e..e1378b9a612 100644
> --- a/io/net-listener.c
> +++ b/io/net-listener.c
> @@ -106,6 +106,45 @@ int qio_net_listener_open_sync(QIONetListener *listener,
> }
> }
> void qio_net_listener_add(QIONetListener *listener,
> QIOChannelSocket *sioc)
> @@ -125,17 +164,7 @@ void qio_net_listener_add(QIONetListener *listener,
> object_ref(OBJECT(sioc));
> listener->connected = true;
>
> - if (listener->io_func != NULL) {
> - trace_qio_net_listener_watch_enabled(listener, listener->io_func,
> - "add");
> - object_ref(OBJECT(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, listener->context);
> - }
> -
> - listener->nsioc++;
> + qio_net_listener_watch(listener, listener->nsioc++, "add");
Nit-picking, I'd have a slight preference to keep the 'nsioc' increment
on the following line from the qio_net_listener_watch call, as I don't
like side effects in passing the function arguments.
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 :|
Am 04.11.2025 um 12:03 hat Daniel P. Berrangé geschrieben:
> On Mon, Nov 03, 2025 at 02:10:55PM -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>
> > ---
> > io/net-listener.c | 109 +++++++++++++++++++---------------------------
> > 1 file changed, 45 insertions(+), 64 deletions(-)
> >
> > diff --git a/io/net-listener.c b/io/net-listener.c
> > index 15df673fb6e..e1378b9a612 100644
> > --- a/io/net-listener.c
> > +++ b/io/net-listener.c
> > @@ -106,6 +106,45 @@ int qio_net_listener_open_sync(QIONetListener *listener,
> > }
> > }
>
> > void qio_net_listener_add(QIONetListener *listener,
> > QIOChannelSocket *sioc)
> > @@ -125,17 +164,7 @@ void qio_net_listener_add(QIONetListener *listener,
> > object_ref(OBJECT(sioc));
> > listener->connected = true;
> >
> > - if (listener->io_func != NULL) {
> > - trace_qio_net_listener_watch_enabled(listener, listener->io_func,
> > - "add");
> > - object_ref(OBJECT(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, listener->context);
> > - }
> > -
> > - listener->nsioc++;
> > + qio_net_listener_watch(listener, listener->nsioc++, "add");
>
> Nit-picking, I'd have a slight preference to keep the 'nsioc' increment
> on the following line from the qio_net_listener_watch call, as I don't
> like side effects in passing the function arguments.
It actually wouldn't work any more because qio_net_listener_watch()
iterates up to listener->nsioc. It needs the increased value in
listener->nsioc, and the previous one for i, so that we get exactly one
loop iteration.
Kevin
On Tue, Nov 04, 2025 at 02:15:16PM +0100, Kevin Wolf wrote: > Am 04.11.2025 um 12:03 hat Daniel P. Berrangé geschrieben: > > On Mon, Nov 03, 2025 at 02:10:55PM -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. > > > > > > - listener->nsioc++; > > > + qio_net_listener_watch(listener, listener->nsioc++, "add"); > > > > Nit-picking, I'd have a slight preference to keep the 'nsioc' increment > > on the following line from the qio_net_listener_watch call, as I don't > > like side effects in passing the function arguments. > > It actually wouldn't work any more because qio_net_listener_watch() > iterates up to listener->nsioc. It needs the increased value in > listener->nsioc, and the previous one for i, so that we get exactly one > loop iteration. I'm happy to change it to this in v2, to avoid the side-effect in the function call: listener->nsioc++; qio_net_listener_watch(listener, listener->nsioc - 1, "add"); -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
© 2016 - 2025 Red Hat, Inc.