[2/2] libcamera: log: add logTargetCros for ChromeOS logs

21611 diff mbox series

From: Han-Lin Chen <hanlinchen@chromium.org>

ChromeOS logging requires that the logs export to both syslog and
std::cerr. Add a logTargetCros for the specific case.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/logging.h |  1 +
 src/libcamera/base/log.cpp  | 61 +++++++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 9 deletions(-)

Hi Harvey, Han-Lin,

Thank you for the patch.

On Mon, Oct 14, 2024 at 08:13:55AM +0000, Harvey Yang wrote:
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> ChromeOS logging requires that the logs export to both syslog and
> std::cerr. Add a logTargetCros for the specific case.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  include/libcamera/logging.h |  1 +
>  src/libcamera/base/log.cpp  | 61 +++++++++++++++++++++++++++++++------
>  2 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h
> index e1c6341ce..b88acf23b 100644
> --- a/include/libcamera/logging.h
> +++ b/include/libcamera/logging.h
> @@ -16,6 +16,7 @@ enum LoggingTarget {
>  	LoggingTargetSyslog,
>  	LoggingTargetFile,
>  	LoggingTargetStream,
> +	LoggingTargetCros,

I'm not a big fan of this being exposed in the public API :-(

>  };
>  
>  int logSetFile(const char *path, bool color = false);
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 3a656b8f0..930e2329e 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -107,6 +107,7 @@ class LogOutput
>  public:
>  	LogOutput(const char *path, bool color);
>  	LogOutput(std::ostream *stream, bool color);
> +	LogOutput(bool color);
>  	LogOutput();
>  	~LogOutput();
>  
> @@ -144,6 +145,19 @@ LogOutput::LogOutput(std::ostream *stream, bool color)
>  {
>  }
>  
> +/**
> + * \brief Construct a log output to both std:err and syslog

s/std:err/std::cerr/

> + * \param[in] color True to output colored messages
> + *
> + * Currently this is only needed for CrOS. Therefore, the target is set to
> + * `LoggingTargetCros`.
> + */
> +LogOutput::LogOutput(bool color)
> +	: stream_(&std::cerr), target_(LoggingTargetCros), color_(color)
> +{
> +	openlog("libcamera", LOG_PID, 0);
> +}

Brainstorming mode, I wonder if it wouldn't be cleaner to add support
for multiple LogOutput instances. That may be overkill though.

> +
>  /**
>   * \brief Construct a log output to syslog
>   */
> @@ -160,6 +174,7 @@ LogOutput::~LogOutput()
>  		delete stream_;
>  		break;
>  	case LoggingTargetSyslog:
> +	case LoggingTargetCros:
>  		closelog();
>  		break;
>  	default:
> @@ -230,17 +245,26 @@ void LogOutput::write(const LogMessage &msg)
>  			severityColor = kColorBrightWhite;
>  	}
>  
> +	bool toStream = false;
> +	bool toSyslog = false;
> +
>  	switch (target_) {
>  	case LoggingTargetSyslog:
> -		str = std::string(log_severity_name(severity)) + " "
> -		    + msg.category().name() + " " + msg.fileInfo() + " ";
> -		if (!msg.prefix().empty())
> -			str += msg.prefix() + ": ";
> -		str += msg.msg();
> -		writeSyslog(severity, str);
> +		toSyslog = true;
>  		break;
>  	case LoggingTargetStream:
>  	case LoggingTargetFile:
> +		toStream = true;
> +		break;
> +	case LoggingTargetCros:
> +		toSyslog = true;
> +		toStream = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (toStream) {
>  		str = "[" + utils::time_point_to_string(msg.timestamp()) + "] ["
>  		    + std::to_string(Thread::currentId()) + "] "
>  		    + severityColor + log_severity_name(severity) + " "
> @@ -250,9 +274,15 @@ void LogOutput::write(const LogMessage &msg)
>  			str += prefixColor + msg.prefix() + ": ";
>  		str += resetColor + msg.msg();
>  		writeStream(str);
> -		break;
> -	default:
> -		break;
> +	}
> +
> +	if (toSyslog) {
> +		str = std::string(log_severity_name(severity)) + " "
> +		    + msg.category().name() + " " + msg.fileInfo() + " ";
> +		if (!msg.prefix().empty())
> +			str += msg.prefix() + ": ";
> +		str += msg.msg();
> +		writeSyslog(severity, str);
>  	}
>  }
>  
> @@ -270,6 +300,10 @@ void LogOutput::write(const std::string &str)
>  	case LoggingTargetFile:
>  		writeStream(str);
>  		break;
> +	case LoggingTargetCros:
> +		writeSyslog(LogDebug, str);
> +		writeStream(str);
> +		break;
>  	default:
>  		break;
>  	}
> @@ -549,6 +583,9 @@ int Logger::logSetTarget(enum LoggingTarget target)
>  	case LoggingTargetNone:
>  		std::atomic_store(&output_, std::shared_ptr<LogOutput>());
>  		break;
> +	case LoggingTargetCros:
> +		std::atomic_store(&output_, std::make_shared<LogOutput>(true));
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -586,6 +623,12 @@ void Logger::logSetLevel(const char *category, const char *level)
>   */
>  Logger::Logger()
>  {
> +#if defined(OS_CHROMEOS)
> +	logSetTarget(LoggingTargetCros);
> +	parseLogLevels();
> +	return;
> +#endif

I'd like to avoid an #ifdef here. Is there a way to expose this feature
through the libcamera API in a clean way, and select the right target
using the API from the HAL code ?

> +
>  	bool color = !utils::secure_getenv("LIBCAMERA_LOG_NO_COLOR");
>  	logSetStream(&std::cerr, color);
>

Hi Laurent,

On Tue, Oct 15, 2024 at 6:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Harvey, Han-Lin,
>
> Thank you for the patch.
>
> On Mon, Oct 14, 2024 at 08:13:55AM +0000, Harvey Yang wrote:
> > From: Han-Lin Chen <hanlinchen@chromium.org>
> >
> > ChromeOS logging requires that the logs export to both syslog and
> > std::cerr. Add a logTargetCros for the specific case.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  include/libcamera/logging.h |  1 +
> >  src/libcamera/base/log.cpp  | 61 +++++++++++++++++++++++++++++++------
> >  2 files changed, 53 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h
> > index e1c6341ce..b88acf23b 100644
> > --- a/include/libcamera/logging.h
> > +++ b/include/libcamera/logging.h
> > @@ -16,6 +16,7 @@ enum LoggingTarget {
> >       LoggingTargetSyslog,
> >       LoggingTargetFile,
> >       LoggingTargetStream,
> > +     LoggingTargetCros,
>
> I'm not a big fan of this being exposed in the public API :-(

Sure, but as you can see, this CL tries to enable multiple
outputs. The naming can be discussed.

>
> >  };
> >
> >  int logSetFile(const char *path, bool color = false);
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index 3a656b8f0..930e2329e 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -107,6 +107,7 @@ class LogOutput
> >  public:
> >       LogOutput(const char *path, bool color);
> >       LogOutput(std::ostream *stream, bool color);
> > +     LogOutput(bool color);
> >       LogOutput();
> >       ~LogOutput();
> >
> > @@ -144,6 +145,19 @@ LogOutput::LogOutput(std::ostream *stream, bool color)
> >  {
> >  }
> >
> > +/**
> > + * \brief Construct a log output to both std:err and syslog
>
> s/std:err/std::cerr/
>
> > + * \param[in] color True to output colored messages
> > + *
> > + * Currently this is only needed for CrOS. Therefore, the target is set to
> > + * `LoggingTargetCros`.
> > + */
> > +LogOutput::LogOutput(bool color)
> > +     : stream_(&std::cerr), target_(LoggingTargetCros), color_(color)
> > +{
> > +     openlog("libcamera", LOG_PID, 0);
> > +}
>
> Brainstorming mode, I wonder if it wouldn't be cleaner to add support
> for multiple LogOutput instances. That may be overkill though.

True, that's an approach.

>
> > +
> >  /**
> >   * \brief Construct a log output to syslog
> >   */
> > @@ -160,6 +174,7 @@ LogOutput::~LogOutput()
> >               delete stream_;
> >               break;
> >       case LoggingTargetSyslog:
> > +     case LoggingTargetCros:
> >               closelog();
> >               break;
> >       default:
> > @@ -230,17 +245,26 @@ void LogOutput::write(const LogMessage &msg)
> >                       severityColor = kColorBrightWhite;
> >       }
> >
> > +     bool toStream = false;
> > +     bool toSyslog = false;
> > +
> >       switch (target_) {
> >       case LoggingTargetSyslog:
> > -             str = std::string(log_severity_name(severity)) + " "
> > -                 + msg.category().name() + " " + msg.fileInfo() + " ";
> > -             if (!msg.prefix().empty())
> > -                     str += msg.prefix() + ": ";
> > -             str += msg.msg();
> > -             writeSyslog(severity, str);
> > +             toSyslog = true;
> >               break;
> >       case LoggingTargetStream:
> >       case LoggingTargetFile:
> > +             toStream = true;
> > +             break;
> > +     case LoggingTargetCros:
> > +             toSyslog = true;
> > +             toStream = true;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     if (toStream) {
> >               str = "[" + utils::time_point_to_string(msg.timestamp()) + "] ["
> >                   + std::to_string(Thread::currentId()) + "] "
> >                   + severityColor + log_severity_name(severity) + " "
> > @@ -250,9 +274,15 @@ void LogOutput::write(const LogMessage &msg)
> >                       str += prefixColor + msg.prefix() + ": ";
> >               str += resetColor + msg.msg();
> >               writeStream(str);
> > -             break;
> > -     default:
> > -             break;
> > +     }
> > +
> > +     if (toSyslog) {
> > +             str = std::string(log_severity_name(severity)) + " "
> > +                 + msg.category().name() + " " + msg.fileInfo() + " ";
> > +             if (!msg.prefix().empty())
> > +                     str += msg.prefix() + ": ";
> > +             str += msg.msg();
> > +             writeSyslog(severity, str);
> >       }
> >  }
> >
> > @@ -270,6 +300,10 @@ void LogOutput::write(const std::string &str)
> >       case LoggingTargetFile:
> >               writeStream(str);
> >               break;
> > +     case LoggingTargetCros:
> > +             writeSyslog(LogDebug, str);
> > +             writeStream(str);
> > +             break;
> >       default:
> >               break;
> >       }
> > @@ -549,6 +583,9 @@ int Logger::logSetTarget(enum LoggingTarget target)
> >       case LoggingTargetNone:
> >               std::atomic_store(&output_, std::shared_ptr<LogOutput>());
> >               break;
> > +     case LoggingTargetCros:
> > +             std::atomic_store(&output_, std::make_shared<LogOutput>(true));
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -586,6 +623,12 @@ void Logger::logSetLevel(const char *category, const char *level)
> >   */
> >  Logger::Logger()
> >  {
> > +#if defined(OS_CHROMEOS)
> > +     logSetTarget(LoggingTargetCros);
> > +     parseLogLevels();
> > +     return;
> > +#endif
>
> I'd like to avoid an #ifdef here. Is there a way to expose this feature
> through the libcamera API in a clean way, and select the right target
> using the API from the HAL code ?

We're trying not to use environment variables in CrOS, as there will
be some confusion and mis-usages.

It's not a critical patch though. If we couldn't find a good approach,
we can skip this. CrOS just keep this in our own branches.

BR,
Harvey

>
> > +
> >       bool color = !utils::secure_getenv("LIBCAMERA_LOG_NO_COLOR");
> >       logSetStream(&std::cerr, color);
> >
>
> --
> Regards,
>
> Laurent Pinchart