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

On Fri, Jun 19, 2026 at 01:10:44PM +0200, Stefano Brivio wrote:

On Wed, 17 Jun 2026 13:11:16 +1000 David Gibson 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