[RFC PATCH v2 7/9] libcamera: base: log: Split `parseLogLevel[s]()`

[Barnabás Pőcze]

Hi


2025. február 6., csütörtök 17:48 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> Hi Barnab at s,
> 
> Thank you for the patch.
> 
> On Thu, Jan 30, 2025 at 07:58:52PM +0000, Barnabás Pőcze wrote:
> > These two functions do not need to be exposed in any
> > way, nor do they need to be a member of the `Logger` class.
> 
> For parseLogLevels() this is not actually true. The function accesses a
> member variable, which you now pass it as a function argument. What's
> the advantage of this patch ?

Ahh, you're right. The motivation is to decouple the log level specification string
source (the environment variable) and destination (`levels_` list) from the
parsing itself. This was done mostly because I was considering potential solutions
to https://bugs.libcamera.org/show_bug.cgi?id=243. Another thing was the introduction
of the lock in the next patch, keeping the number of places where a mutex protected
object is accessed seems like a good idea (since it is not enforced by any kind
of wrapper type) (although since `parseLogLevels()` is only called from
the constructor, I opted not to do any locking there in the next patch).


> 
> > So move them into an anonymous namespace.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  src/libcamera/base/log.cpp | 186 ++++++++++++++++++-------------------
> >  1 file changed, 93 insertions(+), 93 deletions(-)
> >
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index 0dfdb0e9b..19fc5cc67 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -65,7 +65,9 @@
> >
> >  namespace libcamera {
> >
> > -static int log_severity_to_syslog(LogSeverity severity)
> > +namespace {
> > +
> > +int log_severity_to_syslog(LogSeverity severity)
> >  {
> >  	switch (severity) {
> >  	case LogDebug:
> > @@ -83,7 +85,7 @@ static int log_severity_to_syslog(LogSeverity severity)
> >  	}
> >  }
> >
> > -static const char *log_severity_name(LogSeverity severity)
> > +const char *log_severity_name(LogSeverity severity)
> >  {
> >  	static const char *const names[] = {
> >  		"DEBUG",
> > @@ -99,6 +101,92 @@ static const char *log_severity_name(LogSeverity severity)
> >  		return "UNKWN";
> >  }
> >
> > +/**
> > + * \brief Parse a log level string into a LogSeverity
> > + * \param[in] level The log level string
> > + *
> > + * Log levels can be specified as an integer value in the range from LogDebug to
> > + * LogFatal, or as a string corresponding to the severity name in uppercase. Any
> > + * other value is invalid.
> > + *
> > + * \return The log severity, or LogInvalid if the string is invalid
> > + */
> > +LogSeverity parseLogLevel(std::string_view level)
> > +{
> > +	static const char *const names[] = {
> > +		"DEBUG",
> > +		"INFO",
> > +		"WARN",
> > +		"ERROR",
> > +		"FATAL",
> > +	};
> > +
> > +	unsigned int severity;
> > +
> > +	if (std::isdigit(level[0])) {
> > +		auto [end, ec] = std::from_chars(level.data(), level.data() + level.size(), severity);
> > +		if (ec != std::errc() || *end != '\0' || severity > LogFatal)
> > +			severity = LogInvalid;
> > +	} else {
> > +		severity = LogInvalid;
> > +		for (unsigned int i = 0; i < std::size(names); ++i) {
> > +			if (names[i] == level) {
> > +				severity = i;
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	return static_cast<LogSeverity>(severity);
> > +}
> > +
> > +/**
> > + * \brief Parse the log levels from the environment
> 
> Missing \param


ACK


> 
> > + *
> > + * The log levels are stored in the LIBCAMERA_LOG_LEVELS environment variable
> > + * as a list of "category:level" pairs, separated by commas (','). Parse the
> > + * variable and store the levels to configure all log categories.
> > + */
> > +void parseLogLevels(const char *line, std::list<std::pair<std::string, LogSeverity>> &levels)
> > +{
> > +	for (const char *pair = line; *line != '\0'; pair = line) {
> > +		const char *comma = strchrnul(line, ',');
> > +		size_t len = comma - pair;
> > +
> > +		/* Skip over the comma. */
> > +		line = *comma == ',' ? comma + 1 : comma;
> > +
> > +		/* Skip to the next pair if the pair is empty. */
> > +		if (!len)
> > +			continue;
> > +
> > +		std::string_view category;
> > +		std::string_view level;
> > +
> > +		const char *colon = static_cast<const char *>(memchr(pair, ':', len));
> > +		if (!colon) {
> > +			/* 'x' is a shortcut for '*:x'. */
> > +			category = "*";
> > +			level = std::string_view(pair, len);
> > +		} else {
> > +			category = std::string_view(pair, colon - pair);
> > +			level = std::string_view(colon + 1, comma - colon - 1);
> > +		}
> > +
> > +		/* Both the category and the level must be specified. */
> > +		if (category.empty() || level.empty())
> > +			continue;
> > +
> > +		LogSeverity severity = parseLogLevel(level);
> > +		if (severity == LogInvalid)
> > +			continue;
> > +
> > +		levels.emplace_back(category, severity);
> > +	}
> > +}
> > +
> > +} /* namespace */
> > +
> >  /**
> >   * \brief Log output
> >   *
> > @@ -312,8 +400,6 @@ private:
> >  	Logger();
> >
> >  	void parseLogFile();
> > -	void parseLogLevels();
> > -	static LogSeverity parseLogLevel(std::string_view level);
> >
> >  	friend LogCategory;
> >  	void registerCategory(LogCategory *category);
> > @@ -592,7 +678,9 @@ Logger::Logger()
> >  	logSetStream(&std::cerr, color);
> >
> >  	parseLogFile();
> > -	parseLogLevels();
> > +
> > +	if (const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS"))
> > +		parseLogLevels(debug, levels_);
> >  }
> >
> >  /**
> > @@ -618,94 +706,6 @@ void Logger::parseLogFile()
> >  	logSetFile(file, false);
> >  }
> >
> > -/**
> > - * \brief Parse the log levels from the environment
> > - *
> > - * The log levels are stored in the LIBCAMERA_LOG_LEVELS environment variable
> > - * as a list of "category:level" pairs, separated by commas (','). Parse the
> > - * variable and store the levels to configure all log categories.
> > - */
> > -void Logger::parseLogLevels()
> > -{
> > -	const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
> > -	if (!debug)
> > -		return;
> > -
> > -	for (const char *pair = debug; *debug != '\0'; pair = debug) {
> > -		const char *comma = strchrnul(debug, ',');
> > -		size_t len = comma - pair;
> > -
> > -		/* Skip over the comma. */
> > -		debug = *comma == ',' ? comma + 1 : comma;
> > -
> > -		/* Skip to the next pair if the pair is empty. */
> > -		if (!len)
> > -			continue;
> > -
> > -		std::string_view category;
> > -		std::string_view level;
> > -
> > -		const char *colon = static_cast<const char *>(memchr(pair, ':', len));
> > -		if (!colon) {
> > -			/* 'x' is a shortcut for '*:x'. */
> > -			category = "*";
> > -			level = std::string_view(pair, len);
> > -		} else {
> > -			category = std::string_view(pair, colon - pair);
> > -			level = std::string_view(colon + 1, comma - colon - 1);
> > -		}
> > -
> > -		/* Both the category and the level must be specified. */
> > -		if (category.empty() || level.empty())
> > -			continue;
> > -
> > -		LogSeverity severity = parseLogLevel(level);
> > -		if (severity == LogInvalid)
> > -			continue;
> > -
> > -		levels_.emplace_back(category, severity);
> > -	}
> > -}
> > -
> > -/**
> > - * \brief Parse a log level string into a LogSeverity
> > - * \param[in] level The log level string
> > - *
> > - * Log levels can be specified as an integer value in the range from LogDebug to
> > - * LogFatal, or as a string corresponding to the severity name in uppercase. Any
> > - * other value is invalid.
> > - *
> > - * \return The log severity, or LogInvalid if the string is invalid
> > - */
> > -LogSeverity Logger::parseLogLevel(std::string_view level)
> > -{
> > -	static const char *const names[] = {
> > -		"DEBUG",
> > -		"INFO",
> > -		"WARN",
> > -		"ERROR",
> > -		"FATAL",
> > -	};
> > -
> > -	unsigned int severity;
> > -
> > -	if (std::isdigit(level[0])) {
> > -		auto [end, ec] = std::from_chars(level.data(), level.data() + level.size(), severity);
> > -		if (ec != std::errc() || *end != '\0' || severity > LogFatal)
> > -			severity = LogInvalid;
> > -	} else {
> > -		severity = LogInvalid;
> > -		for (unsigned int i = 0; i < std::size(names); ++i) {
> > -			if (names[i] == level) {
> > -				severity = i;
> > -				break;
> > -			}
> > -		}
> > -	}
> > -
> > -	return static_cast<LogSeverity>(severity);
> > -}
> > -
> >  /**
> >   * \brief Register a log category with the logger
> >   * \param[in] category The log category
> 
> --
> Regards,
> 
> Laurent Pinchart
>