Upcoming patches will adjust how net_listener watches for new client
connections; adding trace points now makes it easier to debug that the
changes work as intended. For example, adding
--trace='qio_net_listener*' to the qemu-storage-daemon command line
before --nbd-server will track when the server first starts listening
for clients.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
io/net-listener.c | 17 +++++++++++++++++
io/trace-events | 5 +++++
2 files changed, 22 insertions(+)
diff --git a/io/net-listener.c b/io/net-listener.c
index 47405965a66..0adbc409cf2 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -23,6 +23,7 @@
#include "io/dns-resolver.h"
#include "qapi/error.h"
#include "qemu/module.h"
+#include "trace.h"
QIONetListener *qio_net_listener_new(void)
{
@@ -50,6 +51,7 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
return TRUE;
}
+ trace_qio_net_listener_callback(listener, listener->io_func);
if (listener->io_func) {
listener->io_func(listener, sioc, listener->io_data);
}
@@ -124,6 +126,8 @@ void qio_net_listener_add(QIONetListener *listener,
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,
@@ -143,6 +147,9 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
{
size_t i;
+ if (listener->io_func) {
+ trace_qio_net_listener_watch_disabled(listener, "set_client_func");
+ }
if (listener->io_notify) {
listener->io_notify(listener->io_data);
}
@@ -159,6 +166,8 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
}
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(
@@ -218,6 +227,9 @@ 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]);
@@ -248,6 +260,8 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
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(
@@ -268,6 +282,9 @@ void qio_net_listener_disconnect(QIONetListener *listener)
return;
}
+ if (listener->io_func) {
+ trace_qio_net_listener_watch_disabled(listener, "disconnect");
+ }
for (i = 0; i < listener->nsioc; i++) {
if (listener->io_source[i]) {
g_source_destroy(listener->io_source[i]);
diff --git a/io/trace-events b/io/trace-events
index dc3a63ba1fb..8cc4cae3a5d 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -72,3 +72,8 @@ qio_channel_command_new_pid(void *ioc, int writefd, int readfd, int pid) "Comman
qio_channel_command_new_spawn(void *ioc, const char *binary, int flags) "Command new spawn ioc=%p binary=%s flags=%d"
qio_channel_command_abort(void *ioc, int pid) "Command abort ioc=%p pid=%d"
qio_channel_command_wait(void *ioc, int pid, int ret, int status) "Command abort ioc=%p pid=%d ret=%d status=%d"
+
+# net-listener.c
+qio_net_listener_watch_enabled(void *listener, void *func, const char *extra) "Net listener=%p watch enabled func=%p by %s"
+qio_net_listener_watch_disabled(void *listener, const char *extra) "Net listener=%p watch disabled by %s"
+qio_net_listener_callback(void *listener, void *func) "Net listener=%p callback forwarding to func=%p"
--
2.51.1Am 03.11.2025 um 21:10 hat Eric Blake geschrieben:
> Upcoming patches will adjust how net_listener watches for new client
> connections; adding trace points now makes it easier to debug that the
> changes work as intended. For example, adding
> --trace='qio_net_listener*' to the qemu-storage-daemon command line
> before --nbd-server will track when the server first starts listening
> for clients.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> io/net-listener.c | 17 +++++++++++++++++
> io/trace-events | 5 +++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/io/net-listener.c b/io/net-listener.c
> index 47405965a66..0adbc409cf2 100644
> --- a/io/net-listener.c
> +++ b/io/net-listener.c
> @@ -23,6 +23,7 @@
> #include "io/dns-resolver.h"
> #include "qapi/error.h"
> #include "qemu/module.h"
> +#include "trace.h"
>
> QIONetListener *qio_net_listener_new(void)
> {
> @@ -50,6 +51,7 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
> return TRUE;
> }
>
> + trace_qio_net_listener_callback(listener, listener->io_func);
> if (listener->io_func) {
> listener->io_func(listener, sioc, listener->io_data);
> }
Not necessarily a problem, but I wonder why you decided to have the
trace point unconditional here...
> @@ -143,6 +147,9 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
> {
> size_t i;
>
> + if (listener->io_func) {
> + trace_qio_net_listener_watch_disabled(listener, "set_client_func");
> + }
> if (listener->io_notify) {
> listener->io_notify(listener->io_data);
> }
...while everywhere else you only call it with a non-NULL
listener->io_func.
Kevin
On Tue, Nov 04, 2025 at 12:08:42PM +0100, Kevin Wolf wrote:
> Am 03.11.2025 um 21:10 hat Eric Blake geschrieben:
> > Upcoming patches will adjust how net_listener watches for new client
> > connections; adding trace points now makes it easier to debug that the
> > changes work as intended. For example, adding
> > --trace='qio_net_listener*' to the qemu-storage-daemon command line
> > before --nbd-server will track when the server first starts listening
> > for clients.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > io/net-listener.c | 17 +++++++++++++++++
> > io/trace-events | 5 +++++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/io/net-listener.c b/io/net-listener.c
> > index 47405965a66..0adbc409cf2 100644
> > --- a/io/net-listener.c
> > +++ b/io/net-listener.c
> > @@ -23,6 +23,7 @@
> > #include "io/dns-resolver.h"
> > #include "qapi/error.h"
> > #include "qemu/module.h"
> > +#include "trace.h"
> >
> > QIONetListener *qio_net_listener_new(void)
> > {
> > @@ -50,6 +51,7 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
> > return TRUE;
> > }
> >
> > + trace_qio_net_listener_callback(listener, listener->io_func);
> > if (listener->io_func) {
> > listener->io_func(listener, sioc, listener->io_data);
> > }
>
> Not necessarily a problem, but I wonder why you decided to have the
> trace point unconditional here...
>
> > @@ -143,6 +147,9 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
> > {
> > size_t i;
> >
> > + if (listener->io_func) {
> > + trace_qio_net_listener_watch_disabled(listener, "set_client_func");
> > + }
> > if (listener->io_notify) {
> > listener->io_notify(listener->io_data);
> > }
>
> ...while everywhere else you only call it with a non-NULL
> listener->io_func.
In the former, the trace is printing out the address of which io_func
(including NULL) is currently registered when the callback is reached.
In the case where a single NetListener registers more than one socket
address, but the user uninstalls their callback after the first client
to connect, it is still possible that other pending connections have
still raced, at which point the qio_net_listener_channel_func can
still fire on those later connections even though there is no longer
an io_func from the user. In all the latter cases, I was merely
tracing when state transitioned between the user installing a handler
or removing one.
Unless you think it would be to noisy, I'm game for changing this in
v2 to have all of the traces be unconditional. It is potentially
noisier, but also would aid in spotting how many times a client
requests no change to the current io_func or lack thereof.
Also worth thinking about: in this patch, I added new traces under the
names enabled/disabled, with the trace points being reacable from
multiple locations. Then in 4/8, when refactoring to consolidate
duplicate code, the trace points are reduced to a single usage in
functions named watch/unwatch. It may be worth rethinking naming to
have the tracepoint and function names share the same terminology.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Am 05.11.2025 um 18:18 hat Eric Blake geschrieben:
> On Tue, Nov 04, 2025 at 12:08:42PM +0100, Kevin Wolf wrote:
> > Am 03.11.2025 um 21:10 hat Eric Blake geschrieben:
> > > Upcoming patches will adjust how net_listener watches for new client
> > > connections; adding trace points now makes it easier to debug that the
> > > changes work as intended. For example, adding
> > > --trace='qio_net_listener*' to the qemu-storage-daemon command line
> > > before --nbd-server will track when the server first starts listening
> > > for clients.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > > io/net-listener.c | 17 +++++++++++++++++
> > > io/trace-events | 5 +++++
> > > 2 files changed, 22 insertions(+)
> > >
> > > diff --git a/io/net-listener.c b/io/net-listener.c
> > > index 47405965a66..0adbc409cf2 100644
> > > --- a/io/net-listener.c
> > > +++ b/io/net-listener.c
> > > @@ -23,6 +23,7 @@
> > > #include "io/dns-resolver.h"
> > > #include "qapi/error.h"
> > > #include "qemu/module.h"
> > > +#include "trace.h"
> > >
> > > QIONetListener *qio_net_listener_new(void)
> > > {
> > > @@ -50,6 +51,7 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
> > > return TRUE;
> > > }
> > >
> > > + trace_qio_net_listener_callback(listener, listener->io_func);
> > > if (listener->io_func) {
> > > listener->io_func(listener, sioc, listener->io_data);
> > > }
> >
> > Not necessarily a problem, but I wonder why you decided to have the
> > trace point unconditional here...
> >
> > > @@ -143,6 +147,9 @@ void qio_net_listener_set_client_func_full(QIONetListener *listener,
> > > {
> > > size_t i;
> > >
> > > + if (listener->io_func) {
> > > + trace_qio_net_listener_watch_disabled(listener, "set_client_func");
> > > + }
> > > if (listener->io_notify) {
> > > listener->io_notify(listener->io_data);
> > > }
> >
> > ...while everywhere else you only call it with a non-NULL
> > listener->io_func.
>
> In the former, the trace is printing out the address of which io_func
> (including NULL) is currently registered when the callback is reached.
> In the case where a single NetListener registers more than one socket
> address, but the user uninstalls their callback after the first client
> to connect, it is still possible that other pending connections have
> still raced, at which point the qio_net_listener_channel_func can
> still fire on those later connections even though there is no longer
> an io_func from the user. In all the latter cases, I was merely
> tracing when state transitioned between the user installing a handler
> or removing one.
>
> Unless you think it would be to noisy, I'm game for changing this in
> v2 to have all of the traces be unconditional. It is potentially
> noisier, but also would aid in spotting how many times a client
> requests no change to the current io_func or lack thereof.
I think being noisy in traces is okay. It's easy enough to filter the
output for non-NULL instances if that's what you need. In debugging I
usually want more data, having too much data is rarely a problem.
> Also worth thinking about: in this patch, I added new traces under the
> names enabled/disabled, with the trace points being reacable from
> multiple locations. Then in 4/8, when refactoring to consolidate
> duplicate code, the trace points are reduced to a single usage in
> functions named watch/unwatch. It may be worth rethinking naming to
> have the tracepoint and function names share the same terminology.
Yes, that's a good idea.
Kevin
On Mon, Nov 03, 2025 at 02:10:52PM -0600, Eric Blake wrote: > Upcoming patches will adjust how net_listener watches for new client > connections; adding trace points now makes it easier to debug that the > changes work as intended. For example, adding > --trace='qio_net_listener*' to the qemu-storage-daemon command line > before --nbd-server will track when the server first starts listening > for clients. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > io/net-listener.c | 17 +++++++++++++++++ > io/trace-events | 5 +++++ > 2 files changed, 22 insertions(+) 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 :|
© 2016 - 2025 Red Hat, Inc.