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

22611 diff mbox series

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 replacing the manual matching code
with fnmatch. This has the side effect that more wildcards ("?" and
"[...]") are supported which is acceptable but won't be advertised.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- Replaced matching code with fnmatch instead of a manual fix
---
 src/libcamera/base/log.cpp | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

Hi Stefan,

Thank you for the patch.

On Wed, Jan 22, 2025 at 11:12:19AM +0100, Stefan Klug wrote:
> 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 replacing the manual matching code
> with fnmatch. This has the side effect that more wildcards ("?" and
> "[...]") are supported which is acceptable but won't be advertised.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Replaced matching code with fnmatch instead of a manual fix
> ---
>  src/libcamera/base/log.cpp | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 3a656b8f099f..086103b328e6 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -8,6 +8,7 @@
>  #include <libcamera/base/log.h>
>  
>  #include <array>
> +#include <fnmatch.h>
>  #include <fstream>
>  #include <iostream>
>  #include <list>
> @@ -717,24 +718,9 @@ void Logger::registerCategory(LogCategory *category)
>  	categories_.push_back(category);
>  
>  	const std::string &name = category->name();
> -	for (const std::pair<std::string, LogSeverity> &level : levels_) {
> -		bool match = true;
> -
> -		for (unsigned int i = 0; i < level.first.size(); ++i) {
> -			if (level.first[i] == '*')
> -				break;
> -
> -			if (i >= name.size() ||
> -			    name[i] != level.first[i]) {
> -				match = false;
> -				break;
> -			}
> -		}
> -
> -		if (match) {
> -			category->setSeverity(level.second);
> -			break;
> -		}
> +	for (const auto &[pattern, severity] : levels_) {
> +		if (fnmatch(pattern.c_str(), name.c_str(), FNM_NOESCAPE) == 0)
> +			category->setSeverity(severity);
>  	}

You also need to update the comment at the beginning of the file to
indicate the wildcard can be located anywhere. With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>