On Fri, Nov 17, 2023 at 02:48:25PM +0100, David Marchand wrote: > On Fri, Nov 17, 2023 at 2:27 PM Bruce Richardson > <bruce.richard...@intel.com> wrote: > > > > On Fri, Nov 17, 2023 at 02:18:21PM +0100, David Marchand wrote: > > > Getting readable and consistent logs is important when running a DPDK > > > application, especially when troubleshooting. > > > A common issue with logs is when a DPDK change do not add (or on the > > > contrary add too many \n) in the format string. > > > > > > This issue would only get noticed when actually hitting this log (which > > > may be something difficult to do). > > > > > > This series proposes to introduce a new RTE_LOG helper that is > > > responsible for logging a one line message and spews a build error (with > > > gcc) if any \n is part of the format string. > > > > > > > > > Note: > > > - the first patch is intentionnally sent as a single block: splitting it > > > into per library commits with correct Fixes: tags is a tedious work. > > > I would split it for a non RFC series. For now, it is enough to show > > > case the idea. > > > - the last patch shows how an existing log macro is converted, > > > > > > > > very nice. I definitely think this should be implemented for 24.03 > > I am still wondering how this idea should evolve... > > Some points I have in mind but for which I am not sure what is the best. > > Some log helpers were exposed to applications and had no explicit > requirement wrt \n. > Applications may have used those helpers with multiline messages. > So maybe existing *exposed* helpers should be left untouched... and a > new helper would need to be introduced.
And the existing helpers deprecated. Internal log helpers should not be exposed to apps. > IOW with an example, cryptodev (missing a RTE_ prefix) CDEV_LOG_ERR > macro is publicly exposed. > A CDEV_LOG_LINE_ERR may be needed to avoid breaking external users. > RTE_CDEV_LOG_ERR should be available, though, right? :-) > > There are a lot of other log macros that let it to the callers to add > a trailing \n. > Should we convert them? I would think that, ideally, yes we should. > Converting the *whole* DPDK code to the new helper (with some > exceptions for people who like multiline logs..) would be nice to > close this topic once and for all. > But it would likely be a nightmare for later fixes that contain logs > and which could introduce regressions in such logs if the backport > does not take care of re-adding a \n. Good point. I wonder how many backports add new logs, or modify existing ones? Are there any automated checks, or a checklist for backports to enable us to catch these sort of things? Naming could be a problem, but we could look to move over existing logs by using new log macro names, which should help us to catch these. Even if the log names are a bit awkward, if they are kept internal-only, it shouldn't matter much. [Plus we would always be free to do a mass-rename back to the original name in future, once the backport issues are reduced]. /Bruce
