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

[Laurent Pinchart]

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 at 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 at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang at 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);
>  

-- 
Regards,

Laurent Pinchart