Re: [PATCH 1/8] qio: Add trace points to net_listener

[Kevin Wolf <kwolf@redhat.com>]

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