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

[Laurent Pinchart]

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.

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

> >  		if (match) {
> >  			category->setSeverity(level.second);
> >  			break;

-- 
Regards,

Laurent Pinchart