From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=WTEAIaOr; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id DAED25A0262 for ; Fri, 19 Jun 2026 17:27:44 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1781882861; bh=TCZamxvO2JsfvGLWaph7gS8OtzlrDtpgfqL08sp5m2o=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WTEAIaOrxzyGBUC+H+oXBtM3XAXXGApVOOqOj3RbS7p2qnNal0+RcAnR6DGBVipso G8sBDpqwQAfO0hcRkIJaKiZI8gnMnNhK8oPPd90foTzgbDzppbtOAnzBcwOw9Zih6Z BNnTGPjGIXcP2SZHqO3LkfxFYJi2aCRBAo88EjnO+Oy3Vp59JWqaXOFr+YlU8ay19I Gs+JylDS2zrZH0RoOgxrlMRzKhC3EZ5h5dOI1LwgoiUnP0PNcHSZOReY/CvnizCbKT f2b+h7Rm2Y//iY0KS5fbJpODZbDMg1kfEkyjPmXDwe/lOOlX0nXQ+HBNG0DzgtsGI6 /i4cMebFUhmAg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ghhL56d3tz58lv; Sat, 20 Jun 2026 01:27:41 +1000 (AEST) Date: Sat, 20 Jun 2026 01:27:23 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 0/6] Improvements to flow specific logging Message-ID: References: <20260617031122.2086827-1-david@gibson.dropbear.id.au> <20260619131044.30bdf848@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="/vnU5J1qWMlyDxVX" Content-Disposition: inline In-Reply-To: <20260619131044.30bdf848@elisabeth> Message-ID-Hash: CEFTFMYDNGQUNFWOAEH3VDUIJLU7HKB4 X-Message-ID-Hash: CEFTFMYDNGQUNFWOAEH3VDUIJLU7HKB4 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --/vnU5J1qWMlyDxVX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > 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. > >=20 > > 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. > >=20 > > Still, current drafted posted for consideration. > >=20 > > 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. > >=20 > > 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 >=20 > 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 [shado= wVariable] > struct udp_flow *uflow =3D 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 =3D 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: >=20 > --- > /home/sbrivio/passt/udp.c:727:3: > Type: Extra argument to printf format specifier (PRINTF_ARGS) >=20 > /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 ove= rridden 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. >=20 > 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: >=20 > /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, sta= rting from function 'flow_log__' > /home/sbrivio/passt/flow.c:312:4: note: Frame #1: function 'flow_log__' c= alls 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 >=20 > ...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=3D=3Dtrue, but always passes details=3Dfalse 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. --=20 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 --/vnU5J1qWMlyDxVX Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmo1X88ACgkQzQJF27ox 2GdmBg//ZRPHho/AlDXFn7C6uP05SSTpF6XqD3Z4u+TBGCPDapVKM9HPDu7QaLNn 0ZIRl0vM3+8tTMC3EazZnDx/BK3ISv8PC+lmYMyKTrUALc+yIfxIwUTYt7QITlhs xqMRo/IXFrEYxYZmvGXztOuvMEr/eAAgejAwAV0vkTquykzqlqrM4ONj9NTWGdJe LDco3WvzB8Ggxhu4oy9Cz5eS9MrZJ1PAkSL3yvrOKm0Q7qBdKQuuL2grFrwnp8Jg w0eI8rbwxe6fGT+iAbZAjrUsx2YzaCFL54w22bGQdRrfz0qqe8av3MZC3Rzuwyra ZubFxIwzIt1jjghW0EKXhoB8PRa9KPsZyqW5cgnyiWX8GiArYI2Oauev0DTNKt3f OqfeG+VoLwhE9L6ZYyfN9DtsC4z9oNYpAz8h2IzzHSeEOAIkm4wzD9D+WVFs+99Y dohLwF+CBztTMhGt2Vi8wZ4XrWTXKV5rJlFDNHUudd+sg46NVxJ+xS2d8Yb8DQzR HsM24GcFdyLrmk0T/Oj/ZCp2f+UgDEx1ReVv8X5Q6dkupBIOtxJItwVk+USVp3Vf GR5OSKrgc1Avf5GlfVIIhd5650urOY7IQLzvEpu6yG1xggujslZqpE3C5sscxw2g GAo3TY+cSaMXDlzhaOyT0UXEAjoNhyUT9wY56Cpr7FvxSDkYTjo= =hTQ6 -----END PGP SIGNATURE----- --/vnU5J1qWMlyDxVX--