Re: [PATCH v2 0/6] Improvements to flow specific logging

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4940 bytes --]

On Fri, Jun 19, 2026 at 01:10:44PM +0200, Stefano Brivio wrote:
> On Wed, 17 Jun 2026 13:11:16 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > While working on podman bug 23739, I found some deficiences in how and
> > when we report various socket errors related to specific flows.  Here
> > are some preliminary patches to improve that.
> > 
> > I think patches 1..3 are pretty straightforward and safe.  4/4 I think
> > is a good idea in principle - promoting the priority of a number of
> > messages now that we have ratelimiting.  However, plumbing 'now' all
> > the places we need it resuls in a *lot* of churn, so we might want to
> > postpone or split it up.
> > 
> > Still, current drafted posted for consideration.
> > 
> > v2:
> >  * Added patch 2/6 At Stefano's suggestion
> >  * Simplified the errno Handling fix at Stefano's suggestion
> >  * Added patch 6/6 which I noticed while revising 5/6.
> >  * Made a handful of minor corrections based on feedback.
> > 
> > David Gibson (6):
> >   flow: Regularise flow specific logging helpers
> >   flow: Indent flow details messages
> >   flow: Include flow details with higher priority log messages
> >   flow, udp: Fix errno handling in udp_flow_sock()
> >   flow, treewide: Promote priority of selected flow-linked messages
> >   udp: Improve messages for errors getting errors
> 
> I was about to apply this without 6/6 because it leads cppcheck (at
> least my version, but the warning appears obviously correct) to
> complain that:

Argh.  How did I manage to forget to do a checker run *again*.

> ---
> udp.c:583:20: style: Local variable 'uflow' shadows outer variable [shadowVariable]
>   struct udp_flow *uflow = udp_at_sidx(sidx);
>                    ^
> udp.c:576:19: note: Shadowed declaration
>  struct udp_flow *uflow;
>                   ^
> udp.c:583:20: note: Shadow variable
>   struct udp_flow *uflow = udp_at_sidx(sidx);
>                    ^
> ---

Not only is the warning correct, but it pointed out a deeper error: I
can't actually use a flow_log() function here, because we only
sometimes know what flow we're dealing with at this point.  Fixed.

> and Coverity Scan to report this:
> 
> ---
> /home/sbrivio/passt/udp.c:727:3:
>   Type: Extra argument to printf format specifier (PRINTF_ARGS)
> 
> /home/sbrivio/passt/udp.c:727:3:
>   extra_argument: This argument was not used by the format string: "s".
> /home/sbrivio/passt/flow.c:290:2:
>   printf_function: Calling "vsnprintf" which uses a "printf"-style format string. [Note: The source code implementation of the function has been overridden by a builtin model.]
> ---

This one too, points out a different problem - I failed to attach a
printf __attribute__ to the flow log function, which is why the
compiler didn't catch this.  That in turn caught another minor bug
where I missed one call when removing the flow_log_() parameter in 3/6.

> 
> but this warning, coming from 1/6 (oops, I didn't spot that myself),
> reported by clang-tidy, is a bit more fundamental for the whole series:
> 
> /home/sbrivio/passt/flow.c:281:6: error: function 'flow_log__' is within a recursive call chain [misc-no-recursion,-warnings-as-errors]
>   281 | void flow_log__(const struct flow_common *f, int pri, bool perror, bool details,
>       |      ^
> /home/sbrivio/passt/flow.c:281:6: note: example recursive call chain, starting from function 'flow_log__'
> /home/sbrivio/passt/flow.c:312:4: note: Frame #1: function 'flow_log__' calls function 'flow_log__' here:
>   312 |                         flow_log__(f, pri, false, false, state,
>       |                         ^
> /home/sbrivio/passt/flow.c:312:4: note: ... which was the starting point of the recursive call chain; there may be other cycles
> 
> ...so I didn't proceed for the moment. I guess you meant to call
> flow_log() there instead, but I'm not sure I should fix it up like
> that, as I guess you checked some specific stuff with this version
> which might be broken if I just fix it.

Whereas this one *looks* scary, but is basically fine.  The warning is
technically correct - there really is a recursive call here - but it's
at most one level of recursion (the recursive call only happens if
details==true, but always passes details=false to the next layer).
Plus it's a tail call, so it shouldn't even take an extra stack frame,
except maybe with -O0.

I was aware of this when I wrote it, but figured it was cleaner than
adding yet another layer of wrapper function to avoid it.  I didn't
realise that clang-tidy would whinge about the possibility of
recursion.  Anyway, I added a comment and a lint suppression for v3.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]