libcamera: base: Fix log level parsing when multiple categories are listed
diff mbox series

Message ID 20250606083808.1226386-1-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • libcamera: base: Fix log level parsing when multiple categories are listed
Related show

Commit Message

Stefan Klug June 6, 2025, 8:37 a.m. UTC 
For a list of log levels like LIBCAMERA_LOG_LEVELS="CatA:0,CatB:1" only
the severity of the last entry is correctly parsed.

Due to the change of level to a string_view in 24c2caa1c1b3 ("libcamera:
base: log: Use `std::string_view` to avoid some copies") the level is no
longer necessarily null terminated as it is a view on the original data.

Replace the check for a terminating null by a check for the end position
to fix the issue.

Fixes: 24c2caa1c1b3 ("libcamera: base: log: Use `std::string_view` to avoid some copies")
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/base/log.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Barnabás Pőcze June 6, 2025, 9:11 a.m. UTC | #1 
2025. 06. 06. 10:37 keltezéssel, Stefan Klug írta:
> For a list of log levels like LIBCAMERA_LOG_LEVELS="CatA:0,CatB:1" only
> the severity of the last entry is correctly parsed.
> 
> Due to the change of level to a string_view in 24c2caa1c1b3 ("libcamera:
> base: log: Use `std::string_view` to avoid some copies") the level is no
> longer necessarily null terminated as it is a view on the original data.
> 
> Replace the check for a terminating null by a check for the end position
> to fix the issue.

Oops, sorry. I hope it wasn't too hard to find.


> 
> Fixes: 24c2caa1c1b3 ("libcamera: base: log: Use `std::string_view` to avoid some copies")
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/libcamera/base/log.cpp | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 8bf3e1daa9c6..e024b58d7180 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -690,8 +690,9 @@ LogSeverity Logger::parseLogLevel(std::string_view level)
>   	unsigned int severity = LogInvalid;
>   
>   	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)
> +		const char* levelEnd = level.data() + level.size();

const char *levelEnd ?

Could a test be added? Maybe something like this:

---
diff --git a/test/log/log_api.cpp b/test/log/log_api.cpp
index 0b999738d..8587ea30d 100644
--- a/test/log/log_api.cpp
+++ b/test/log/log_api.cpp
@@ -26,10 +26,42 @@ using namespace std;
  using namespace libcamera;
  
  LOG_DEFINE_CATEGORY(LogAPITest)
+LOG_DEFINE_CATEGORY(Cat0)
+LOG_DEFINE_CATEGORY(Cat1)
+LOG_DEFINE_CATEGORY(Cat2)
+LOG_DEFINE_CATEGORY(Cat3)
+LOG_DEFINE_CATEGORY(Cat4)
  
  class LogAPITest : public Test
  {
  protected:
+	int testEnvLevels()
+	{
+		setenv("LIBCAMERA_LOG_LEVELS",
+		       "Cat0:0,Cat0:9999,Cat1:INFO,Cat1:INVALID,Cat2:2,Cat2:-1,Cat3:ERROR,Cat3:{[]},Cat4:4,Cat4:rubbish",
+		       true);
+		logSetTarget(libcamera::LoggingTargetNone);
+
+		const std::pair<const LogCategory &, libcamera::LogSeverity> expected[] = {
+			{ _LOG_CATEGORY(Cat0)(), libcamera::LogDebug },
+			{ _LOG_CATEGORY(Cat1)(), libcamera::LogInfo },
+			{ _LOG_CATEGORY(Cat2)(), libcamera::LogWarning },
+			{ _LOG_CATEGORY(Cat3)(), libcamera::LogError },
+			{ _LOG_CATEGORY(Cat4)(), libcamera::LogFatal },
+		};
+		bool ok = true;
+
+		for (const auto &[c, s] : expected) {
+			if (c.severity() != s) {
+				ok = false;
+				cerr << "Severity of " << c.name() << " (" << c.severity() << ") "
+				     << "does not equal " << s << endl;
+			}
+		}
+
+		return ok ? TestPass : TestFail;
+	}
+
  	void doLogging()
  	{
  		logSetLevel("LogAPITest", "DEBUG");
@@ -135,7 +167,11 @@ protected:
  
  	int run() override
  	{
-		int ret = testFile();
+		int ret = testEnvLevels();
+		if (ret != TestPass)
+			return TestFail;
+
+		ret = testFile();
  		if (ret != TestPass)
  			return TestFail;
  
---

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


> +		auto [end, ec] = std::from_chars(level.data(), levelEnd, severity);
> +		if (ec != std::errc() || end != levelEnd || severity > LogFatal)
>   			severity = LogInvalid;
>   	} else {
>   		for (unsigned int i = 0; i < std::size(names); ++i) {
Stefan Klug June 12, 2025, 2:02 p.m. UTC | #2 
Hi Barnabás,

Thank you for the review.

Quoting Barnabás Pőcze (2025-06-06 11:11:03)
> 2025. 06. 06. 10:37 keltezéssel, Stefan Klug írta:
> > For a list of log levels like LIBCAMERA_LOG_LEVELS="CatA:0,CatB:1" only
> > the severity of the last entry is correctly parsed.
> > 
> > Due to the change of level to a string_view in 24c2caa1c1b3 ("libcamera:
> > base: log: Use `std::string_view` to avoid some copies") the level is no
> > longer necessarily null terminated as it is a view on the original data.
> > 
> > Replace the check for a terminating null by a check for the end position
> > to fix the issue.
> 
> Oops, sorry. I hope it wasn't too hard to find.

No worries, it just took a while to realize that I'm missing some
messages :-)

> 
> 
> > 
> > Fixes: 24c2caa1c1b3 ("libcamera: base: log: Use `std::string_view` to avoid some copies")
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >   src/libcamera/base/log.cpp | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index 8bf3e1daa9c6..e024b58d7180 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -690,8 +690,9 @@ LogSeverity Logger::parseLogLevel(std::string_view level)
> >       unsigned int severity = LogInvalid;
> >   
> >       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)
> > +             const char* levelEnd = level.data() + level.size();
> 
> const char *levelEnd ?

Right, fixed that.

> 
> Could a test be added? Maybe something like this:
> 
> ---
> diff --git a/test/log/log_api.cpp b/test/log/log_api.cpp
> index 0b999738d..8587ea30d 100644
> --- a/test/log/log_api.cpp
> +++ b/test/log/log_api.cpp
> @@ -26,10 +26,42 @@ using namespace std;
>   using namespace libcamera;
>   
>   LOG_DEFINE_CATEGORY(LogAPITest)
> +LOG_DEFINE_CATEGORY(Cat0)
> +LOG_DEFINE_CATEGORY(Cat1)
> +LOG_DEFINE_CATEGORY(Cat2)
> +LOG_DEFINE_CATEGORY(Cat3)
> +LOG_DEFINE_CATEGORY(Cat4)
>   
>   class LogAPITest : public Test
>   {
>   protected:
> +       int testEnvLevels()
> +       {
> +               setenv("LIBCAMERA_LOG_LEVELS",
> +                      "Cat0:0,Cat0:9999,Cat1:INFO,Cat1:INVALID,Cat2:2,Cat2:-1,Cat3:ERROR,Cat3:{[]},Cat4:4,Cat4:rubbish",
> +                      true);
> +               logSetTarget(libcamera::LoggingTargetNone);
> +
> +               const std::pair<const LogCategory &, libcamera::LogSeverity> expected[] = {
> +                       { _LOG_CATEGORY(Cat0)(), libcamera::LogDebug },
> +                       { _LOG_CATEGORY(Cat1)(), libcamera::LogInfo },
> +                       { _LOG_CATEGORY(Cat2)(), libcamera::LogWarning },
> +                       { _LOG_CATEGORY(Cat3)(), libcamera::LogError },
> +                       { _LOG_CATEGORY(Cat4)(), libcamera::LogFatal },
> +               };
> +               bool ok = true;
> +
> +               for (const auto &[c, s] : expected) {
> +                       if (c.severity() != s) {
> +                               ok = false;
> +                               cerr << "Severity of " << c.name() << " (" << c.severity() << ") "
> +                                    << "does not equal " << s << endl;
> +                       }
> +               }
> +
> +               return ok ? TestPass : TestFail;
> +       }
> +
>         void doLogging()
>         {
>                 logSetLevel("LogAPITest", "DEBUG");
> @@ -135,7 +167,11 @@ protected:
>   
>         int run() override
>         {
> -               int ret = testFile();
> +               int ret = testEnvLevels();
> +               if (ret != TestPass)
> +                       return TestFail;
> +
> +               ret = testFile();
>                 if (ret != TestPass)
>                         return TestFail;

Thanks for writing this. I added it as a patch to v2. I hope you are
fine with being tagged as co-author.

Best regards,
Stefan

>   
> ---
> 
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> 
> > +             auto [end, ec] = std::from_chars(level.data(), levelEnd, severity);
> > +             if (ec != std::errc() || end != levelEnd || severity > LogFatal)
> >                       severity = LogInvalid;
> >       } else {
> >               for (unsigned int i = 0; i < std::size(names); ++i) {
>

Patch
diff mbox series 

diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
index 8bf3e1daa9c6..e024b58d7180 100644
--- a/src/libcamera/base/log.cpp
+++ b/src/libcamera/base/log.cpp
@@ -690,8 +690,9 @@  LogSeverity Logger::parseLogLevel(std::string_view level)
 	unsigned int severity = LogInvalid;
 
 	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)
+		const char* levelEnd = level.data() + level.size();
+		auto [end, ec] = std::from_chars(level.data(), levelEnd, severity);
+		if (ec != std::errc() || end != levelEnd || severity > LogFatal)
 			severity = LogInvalid;
 	} else {
 		for (unsigned int i = 0; i < std::size(names); ++i) {