libcamera: log: Match whole category in LIBCAMERA_LOG_LEVELS
diff mbox series

Message ID 20250120133038.817550-1-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • libcamera: log: Match whole category in LIBCAMERA_LOG_LEVELS
Related show

Commit Message

Stefan Klug Jan. 20, 2025, 1:30 p.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/base/log.cpp | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Barnabás Pőcze Jan. 20, 2025, 1:59 p.m. UTC | #1 
Hi


2025. január 20., hétfő 14:30 keltezéssel, Stefan Klug <stefan.klug@ideasonboard.com> í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@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.


Regards,
Barnabás Pőcze


>  		if (match) {
>  			category->setSeverity(level.second);
>  			break;
> --
> 2.43.0
> 
>
Laurent Pinchart Jan. 21, 2025, 11:30 a.m. UTC | #2 
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@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;
Barnabás Pőcze Jan. 21, 2025, 11:42 a.m. UTC | #3 
2025. január 21., kedd 12:30 keltezéssel, Laurent Pinchart <laurent.pinchart@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@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

Patch
diff mbox series 

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;
+
 		if (match) {
 			category->setSeverity(level.second);
 			break;