[PATCH] libcamera: log: Match whole category in LIBCAMERA_LOG_LEVELS

[Barnabás Pőcze]

2025. január 21., kedd 12:30 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> On Mon, Jan 20, 2025 at 01:59:47PM +0000, Barnabás Pőcze wrote:
> > 2025. január 20., hétfő 14:30 keltezéssel, Stefan Klug írta:
> >
> > > A LIBCAMERA_LOG_LEVELS value of "RkISP1:0" also applies to RkISP1Ccm and
> > > RkISP1Awb. This behavior is unexpected as it automatically enables all
> > > algorithm log categories when the intent is only to increase the log
> > > level of the upper category. Fix that by ensuring that the full name
> > > gets matched.  The * wildcard is still supported, so RkISP1* matches
> > > RkISP1 and RkISP1Awb.
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/libcamera/base/log.cpp | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > > index 3a656b8f099f..36e57d6017ab 100644
> > > --- a/src/libcamera/base/log.cpp
> > > +++ b/src/libcamera/base/log.cpp
> > > @@ -718,11 +718,15 @@ void Logger::registerCategory(LogCategory *category)
> > >
> > >  	const std::string &name = category->name();
> > >  	for (const std::pair<std::string, LogSeverity> &level : levels_) {
> > > +		unsigned int i;
> > > +		bool wildcard = false;
> > >  		bool match = true;
> > >
> > > -		for (unsigned int i = 0; i < level.first.size(); ++i) {
> > > -			if (level.first[i] == '*')
> > > +		for (i = 0; i < level.first.size(); ++i) {
> > > +			if (level.first[i] == '*') {
> > > +				wildcard = true;
> > >  				break;
> > > +			}
> > >
> > >  			if (i >= name.size() ||
> > >  			    name[i] != level.first[i]) {
> > > @@ -731,6 +735,10 @@ void Logger::registerCategory(LogCategory *category)
> > >  			}
> > >  		}
> > >
> > > +		/* Ensure the full name got matched */
> > > +		if (!(wildcard || i == name.size()))
> > > +			continue;
> > > +
> >
> > Would `fnmatch()` work? If so, then I think that would simplify the code while
> > allowing a greater set of wildcards.
> 
> That would probably work, and the function is available in all the
> standard libraries we support as far as I can tell. Supporting wildcards
> in other locations than the end of the string seems useful. The
> documentation at the beginning of the file should be updated, it
> currently states that the wildcard character can only be at the end.
> 
> I would prefer not advertising support for '?' and '[...]' for now, but
> I'm fine if they are supported as a side effect.

I agree.


> 
> Could we maybe address https://bugs.libcamera.org/show_bug.cgi?id=243
> while at it ? Barnabás, would you like to volunteer ? :-)

I'll take a look.


Regards,
Barnabás Pőcze


> 
> > >  		if (match) {
> > >  			category->setSeverity(level.second);
> > >  			break;
> 
> --
> Regards,
> 
> Laurent Pinchart