Re: [RFC 0/3] Detect superfluous newline in logs

[Bruce Richardson <bruce.richardson@intel.com>]

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.richardson@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