[libcamera-devel,v2,1/4] libcamera: logging: add syslog, stream, and nowhere logging targets

1707 diff mbox series

Allow logging to syslog, or any given ostream, or to nowhere. The
logging API is updated to accomodate these new logging destinations.
LogMessage is modified to allow this.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v2:
- make LogMessage's message component getters return the data directly,
  and not as a string (eg. severity, category, timestamp)
- move LogMessage's message component formatting (converting to string)
  to Logger
- add LogOutput to abstract away log output destinations from the
  Logger, and to prevent anticipated concurrency issues (changing the
  log output between commencing and completing a previous write)
- upgrade documentation
- other minor changes

 include/libcamera/logging.h |  13 +-
 src/libcamera/include/log.h |   8 +
 src/libcamera/log.cpp       | 458 +++++++++++++++++++++++++++++-------
 3 files changed, 397 insertions(+), 82 deletions(-)

Hi Paul,

Thank you for the patch.

On Tue, Jul 16, 2019 at 04:05:05PM +0900, Paul Elder wrote:
> Allow logging to syslog, or any given ostream, or to nowhere. The
> logging API is updated to accomodate these new logging destinations.
> LogMessage is modified to allow this.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v2:
> - make LogMessage's message component getters return the data directly,
>   and not as a string (eg. severity, category, timestamp)
> - move LogMessage's message component formatting (converting to string)
>   to Logger
> - add LogOutput to abstract away log output destinations from the
>   Logger, and to prevent anticipated concurrency issues (changing the
>   log output between commencing and completing a previous write)
> - upgrade documentation
> - other minor changes
> 
>  include/libcamera/logging.h |  13 +-
>  src/libcamera/include/log.h |   8 +
>  src/libcamera/log.cpp       | 458 +++++++++++++++++++++++++++++-------
>  3 files changed, 397 insertions(+), 82 deletions(-)
> 
> diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h
> index a8495cf..2b6dd3f 100644
> --- a/include/libcamera/logging.h
> +++ b/include/libcamera/logging.h
> @@ -9,8 +9,17 @@
>  
>  namespace libcamera {
>  
> -void logSetFile(const char *file);
> -int logSetLevel(const char *category, const char *level);
> +enum LoggingTarget {
> +	LoggingTargetNone,
> +	LoggingTargetSyslog,
> +	LoggingTargetFile,
> +	LoggingTargetStream,
> +};
> +
> +int logSetFile(const char *path);
> +int logSetStream(std::ostream *stream);
> +int logSetTarget(LoggingTarget target);
> +void logSetLevel(const char *category, const char *level);
>  
>  } /* namespace libcamera */
>  
> diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h
> index 802836d..9b203f9 100644
> --- a/src/libcamera/include/log.h
> +++ b/src/libcamera/include/log.h
> @@ -60,12 +60,20 @@ public:
>  
>  	std::ostream &stream() { return msgStream_; }
>  
> +	const struct timespec &timestamp() const { return timestamp_; }
> +	LogSeverity severity() const { return severity_; }
> +	const LogCategory &category() const { return category_; }
> +	const std::string &fileInfo() const { return fileInfo_; }
> +	const std::string msg() const { return msgStream_.str(); }
> +
>  private:
>  	void init(const char *fileName, unsigned int line);
>  
>  	std::ostringstream msgStream_;
>  	const LogCategory &category_;
>  	LogSeverity severity_;
> +	struct timespec timestamp_;
> +	std::string fileInfo_;
>  };
>  
>  class Loggable
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 709c669..3108ef3 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -15,8 +15,11 @@
>  #include <iostream>
>  #include <list>
>  #include <string.h>
> +#include <syslog.h>
>  #include <unordered_set>
>  
> +#include <libcamera/logging.h>
> +
>  #include "utils.h"
>  
>  /**
> @@ -48,8 +51,172 @@
>   * to stderr.
>   */
>  
> +/**
> + * \file logging.h
> + * \brief Logging management
> + *
> + * API to change the logging output destination and log levels programatically.
> + */
> +
>  namespace libcamera {
>  
> +static int log_severity_to_syslog(LogSeverity severity)
> +{
> +	switch (severity) {
> +	case LogDebug:
> +		return LOG_DEBUG;
> +	case LogInfo:
> +		return LOG_INFO;
> +	case LogWarning:
> +		return LOG_WARNING;
> +	case LogError:
> +		return LOG_ERR;
> +	case LogFatal:
> +		return LOG_ALERT;
> +	default:
> +		return LOG_NOTICE;
> +	}
> +}
> +
> +static std::string log_timespec_to_string(const struct timespec &timestamp)
> +{
> +	std::ostringstream ossTimestamp;
> +	ossTimestamp.fill('0');
> +	ossTimestamp << "[" << timestamp.tv_sec / (60 * 60) << ":"
> +		     << std::setw(2) << (timestamp.tv_sec / 60) % 60 << ":"
> +		     << std::setw(2) << timestamp.tv_sec % 60 << "."
> +		     << std::setw(9) << timestamp.tv_nsec << "]";
> +	ossTimestamp.fill(' ');

I think you can drop this line.

> +	return ossTimestamp.str();
> +}
> +
> +static const char *log_severity_name(LogSeverity severity)
> +{
> +	static const char *const names[] = {
> +		"  DBG",
> +		" INFO",
> +		" WARN",
> +		"  ERR",
> +		"FATAL",
> +	};
> +
> +	if (static_cast<unsigned int>(severity) < ARRAY_SIZE(names))
> +		return names[severity];
> +	else
> +		return "UNKWN";
> +}
> +
> +/**
> + * \brief Log output
> + *
> + * The LogOutput class models a log output desination

s/desination/destination/

> + */
> +class LogOutput
> +{
> +public:
> +	LogOutput(const char *path);
> +	LogOutput(std::ostream *stream);
> +	LogOutput();
> +	~LogOutput();
> +
> +	bool good() const;
> +	void write(const LogMessage &msg);
> +
> +private:
> +	void writeSyslog(const LogMessage &msg);
> +	void writeStream(const LogMessage &msg);
> +
> +	std::ostream *stream_;
> +	LoggingTarget target_;
> +};
> +
> +/**
> + * \brief Construct a log output based on a file
> + * \param[in] path Full path to log file
> + */
> +LogOutput::LogOutput(const char *path)
> +	: target_(LoggingTargetFile)
> +{
> +	stream_ = new std::ofstream(path);
> +}
> +
> +/**
> + * \brief Construct a log output based on a stream
> + * \param[in] stream Stream to send log output to
> + */
> +LogOutput::LogOutput(std::ostream *stream)
> +	: stream_(stream), target_(LoggingTargetStream)
> +{
> +}
> +
> +/**
> + * \brief Construct a log output to syslog
> + */
> +LogOutput::LogOutput()
> +	: stream_(nullptr), target_(LoggingTargetSyslog)
> +{
> +	openlog("libcamera", LOG_PID, 0);
> +}
> +
> +LogOutput::~LogOutput()
> +{
> +	switch (target_) {
> +	case LoggingTargetFile:
> +		delete stream_;
> +		break;
> +	case LoggingTargetSyslog:
> +		closelog();
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +/**
> + * \brief Check if log output is a valid stream

Missing \return, and the documentation doesn't really match the
implementation. I would document it as

 * \brief Check if the log output is valid
 * \return True if the log output is valid

> + */
> +bool LogOutput::good() const

Then I think we can rename this to isValid()

> +{
> +	return stream_ && stream_->good();

And implement it as

	switch (target_) {
	case LoggingTargetFile:
		return stream_->good();
	case LoggingTargetStream:
		return stream_ != nulptr;
	default:
		return true;
	}

> +}
> +
> +/**
> + * \brief Write message to log output
> + * \param[in] msg Message to write
> + */
> +void LogOutput::write(const LogMessage &msg)
> +{
> +	switch (target_) {
> +	case LoggingTargetSyslog:
> +		writeSyslog(msg);
> +		break;
> +	case LoggingTargetStream:
> +	case LoggingTargetFile:
> +		writeStream(msg);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +void LogOutput::writeSyslog(const LogMessage &msg)
> +{
> +	std::string str = std::string(log_severity_name(msg.severity())) + " " +
> +	      		  msg.category().name() + " " + msg.fileInfo() + " " +
> +			  msg.msg();
> +	syslog(log_severity_to_syslog(msg.severity()), "%s", str.c_str());
> +}
> +
> +void LogOutput::writeStream(const LogMessage &msg)
> +{
> +	std::string str = std::string(log_timespec_to_string(msg.timestamp()) +
> +	      		  log_severity_name(msg.severity()) + " " +
> +			  msg.category().name() + " " + msg.fileInfo() + " " +
> +			  msg.msg());
> +	stream_->write(str.c_str(), str.size());
> +	stream_->flush();
> +}
> +
>  /**
>   * \brief Message logger
>   *
> @@ -60,7 +227,12 @@ class Logger
>  public:
>  	static Logger *instance();
>  
> -	void write(const std::string &msg);
> +	void write(const LogMessage &msg);
> +
> +	int logSetFile(const char *path);
> +	int logSetStream(std::ostream *stream);
> +	int logSetTarget(LoggingTarget target);
> +	void logSetLevel(const char *category, const char *level);
>  
>  private:
>  	Logger();
> @@ -69,52 +241,91 @@ private:
>  	void parseLogLevels();
>  	static LogSeverity parseLogLevel(const std::string &level);
>  
> -	friend int logSetFile(const char *file);
> -	friend void logSetLevel(const char *category, const char *level);
> -
>  	friend LogCategory;
>  	void registerCategory(LogCategory *category);
>  	void unregisterCategory(LogCategory *category);
>  
> +	void writeSyslog(const LogMessage &msg);
> +	void writeStream(const LogMessage &msg);
> +
>  	std::unordered_set<LogCategory *> categories_;
>  	std::list<std::pair<std::string, LogSeverity>> levels_;
>  
> -	std::ofstream file_;
> -	std::ostream *output_;
> +	std::shared_ptr<LogOutput> output_;
>  };
>  
>  /**
> - * \brief Set the log file
> - * \param[in] file Full path to the log file
> + * \enum LoggingTarget
> + * \brief Log destination type
> + * \var LoggingTargetNone
> + * \brief No logging destination
> + * \sa Logger::logSetTarget
> + * \var LoggingTargetSyslog
> + * \brief Log to syslog
> + * \sa Logger::logSetTarget
> + * \var LoggingTargetFile
> + * \brief Log to file
> + * \sa Logger::logSetFile
> + * \var LoggingTargetStream
> + * \brief Log to stream
> + * \sa Logger::logSetStream
> + */
> +
> +/**
> + * \brief Direct logging to a file
> + * \param[in] path Full path to the log file
>   *
> - * This function sets the logging output file to \a file. The previous log file,
> - * if any, is closed, and all new log messages will be written to the new log
> - * file.
> + * This function directs the log output to the file identified by \a path. The
> + * previous log target, if any, is closed, and all new log messages will be
> + * written to the new log file.
>   *
> - * If \a file is a null pointer, the log is directed to stderr. If the
> - * function returns an error, the log file is not changed.
> + * If the function returns an error, the log target is not changed.
>   *
>   * \return Zero on success, or a negative error code otherwise.

While at it, s/\.$// (and the same below)

>   */
> -int logSetFile(const char *file)
> +int logSetFile(const char *path)
>  {
> -	Logger *logger = Logger::instance();
> -
> -	if (!file) {
> -		logger->output_ = &std::cerr;
> -		logger->file_.close();
> -		return 0;
> -	}
> +	return Logger::instance()->logSetFile(path);
> +}
>  
> -	std::ofstream logFile(file);
> -	if (!logFile.good())
> -		return -EINVAL;
> +/**
> + * \brief Direct logging to a stream
> + * \param[in] stream Stream to send log output to
> + *
> + * This function directs the log output to \a stream. The previous log target,
> + * if any, is closed, and all new log messages will be written to the new log
> + * stream.
> + *
> + * If the function returns an error, the log file is not changed.
> + *
> + * \return Zero on success, or a negative error code otherwise.
> + */
> +int logSetStream(std::ostream *stream)
> +{
> +	return Logger::instance()->logSetStream(stream);
> +}
>  
> -	if (logger->output_ != &std::cerr)
> -		logger->file_.close();
> -	logger->file_ = std::move(logFile);
> -	logger->output_ = &logger->file_;
> -	return 0;
> +/**
> + * \brief Set the logging target
> + * \param[in] target Logging destination
> + *
> + * This function sets the logging output to the target specified by \a target.
> + * The allowed values of \a target are LoggingTargetNone and
> + * LoggingTargetSyslog. LoggingTargetNone will send the log output to nowhere,
> + * and LoggingTargetSyslog will send the log output to syslog. The previous
> + * log target, if any, is closed, and all new log messages will be written to
> + * the new log destination.
> + *
> + * LoggingTargetFile and LoggingTargetStream are not valid values for \a target.
> + * Use logSetFile() and logSetStream() instead, respectively.
> + *
> + * If the function returns an error, the log file is not changed.
> + *
> + * \return Zero on success, or a negative error code otherwise.
> + */
> +int logSetTarget(LoggingTarget target)
> +{
> +	return Logger::instance()->logSetTarget(target);
>  }
>  
>  /**
> @@ -134,15 +345,7 @@ int logSetFile(const char *file)
>   */
>  void logSetLevel(const char *category, const char *level)
>  {
> -	Logger *logger = Logger::instance();
> -
> -	LogSeverity severity = Logger::parseLogLevel(level);
> -	if (severity == LogInvalid)
> -		return;
> -
> -	for (LogCategory *c : logger->categories_)
> -		if (!strcmp(c->name(), category))
> -			c->setSeverity(severity);
> +	Logger::instance()->logSetLevel(category, level);
>  }
>  
>  /**
> @@ -161,19 +364,103 @@ Logger *Logger::instance()
>  
>  /**
>   * \brief Write a message to the configured logger output
> - * \param[in] msg The message string
> + * \param[in] msg The message object
> + */
> +void Logger::write(const LogMessage &msg)
> +{
> +	std::shared_ptr<LogOutput> output = std::atomic_load(&output_);
> +	if (!output)
> +		return;
> +
> +	output->write(msg);
> +}
> +
> +/**
> + * \brief Set the log file
> + * \param[in] path Full path to the log file
> + *
> + * \sa libcamera::logSetFile()
> + *
> + * \return Zero on success, or a negative error code otherwise.
> + */
> +int Logger::logSetFile(const char *path)
> +{
> +	std::shared_ptr<LogOutput> output = std::make_shared<LogOutput>(path);
> +	if (!output->good())
> +		return -EINVAL;
> +
> +	std::atomic_store(&output_, output);
> +	return 0;
> +}
> +
> +/**
> + * \brief Set the log stream
> + * \param[in] stream Stream to send log output to
> + *
> + * \sa libcamera::logSetStream()
> + *
> + * \return Zero on success, or a negative error code otherwise.
>   */
> -void Logger::write(const std::string &msg)
> +int Logger::logSetStream(std::ostream *stream)
>  {
> -	output_->write(msg.c_str(), msg.size());
> -	output_->flush();
> +	std::shared_ptr<LogOutput> output = std::make_shared<LogOutput>(stream);
> +	std::atomic_store(&output_, output);
> +	return 0;
> +}
> +
> +/**
> + * \brief Set the log target
> + * \param[in] target Log destination
> + *
> + * \sa libcamera::logSetTarget()
> + *
> + * \return Zero on success, or a negative error code otherwise.
> + */
> +int Logger::logSetTarget(enum LoggingTarget target)
> +{
> +	std::shared_ptr<LogOutput> output;
> +
> +	switch (target) {
> +	case LoggingTargetSyslog:
> +		output = std::make_shared<LogOutput>();
> +		std::atomic_store(&output_, output);
> +		break;
> +	case LoggingTargetNone:
> +		output = nullptr;
> +		std::atomic_store(&output_, output);

std::atomic_store(&output_, std::shared_ptr<LogOutput>());

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Set the log level
> + * \param[in] category Logging category
> + * \param[in] level Log level
> + *
> + * \sa libcamera::logSetLevel()
> + */
> +void Logger::logSetLevel(const char *category, const char *level)
> +{
> +	LogSeverity severity = parseLogLevel(level);
> +	if (severity == LogInvalid)
> +		return;
> +
> +	for (LogCategory *c : categories_) {
> +		if (!strcmp(c->name(), category)) {
> +			c->setSeverity(severity);
> +			break;
> +		}
> +	}
>  }
>  
>  /**
>   * \brief Construct a logger
>   */
>  Logger::Logger()
> -	: output_(&std::cerr)
>  {
>  	parseLogFile();
>  	parseLogLevels();
> @@ -183,18 +470,24 @@ Logger::Logger()
>   * \brief Parse the log output file from the environment
>   *
>   * If the LIBCAMERA_LOG_FILE environment variable is set, open the file it
> - * points to and redirect the logger output to it. Errors are silently ignored
> - * and don't affect the logger output (set to stderr).
> + * points to and redirect the logger output to it. If environment variable is

s/If environment/If the environment/

> + * set to "syslog", then the logger output will be directed to syslog. Errors
> + * are silently ignored and don't affect the logger output (set to stderr).
>   */
>  void Logger::parseLogFile()
>  {
>  	const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> -	if (!file)
> +	if (!file) {
> +		logSetStream(&std::cerr);
>  		return;
> +	}
> +
> +	if (!strcmp(file, "syslog")) {
> +		logSetTarget(LoggingTargetSyslog);
> +		return;
> +	}
>  
> -	file_.open(file);
> -	if (file_.good())
> -		output_ = &file_;
> +	logSetFile(file);
>  }
>  
>  /**
> @@ -408,22 +701,6 @@ const LogCategory &LogCategory::defaultCategory()
>  	return category;
>  }
>  
> -static const char *log_severity_name(LogSeverity severity)
> -{
> -	static const char *const names[] = {
> -		"  DBG",
> -		" INFO",
> -		" WARN",
> -		"  ERR",
> -		"FATAL",
> -	};
> -
> -	if (static_cast<unsigned int>(severity) < ARRAY_SIZE(names))
> -		return names[severity];
> -	else
> -		return "UNKWN";
> -}
> -
>  /**
>   * \class LogMessage
>   * \brief Internal log message representation.
> @@ -493,18 +770,11 @@ LogMessage::LogMessage(LogMessage &&other)
>  void LogMessage::init(const char *fileName, unsigned int line)
>  {
>  	/* Log the timestamp, severity and file information. */
> -	struct timespec timestamp;
> -	clock_gettime(CLOCK_MONOTONIC, &timestamp);
> -	msgStream_.fill('0');
> -	msgStream_ << "[" << timestamp.tv_sec / (60 * 60) << ":"
> -		   << std::setw(2) << (timestamp.tv_sec / 60) % 60 << ":"
> -		   << std::setw(2) << timestamp.tv_sec % 60 << "."
> -		   << std::setw(9) << timestamp.tv_nsec << "]";
> -	msgStream_.fill(' ');
> +	clock_gettime(CLOCK_MONOTONIC, &timestamp_);
>  
> -	msgStream_ << " " << log_severity_name(severity_);
> -	msgStream_ << " " << category_.name();
> -	msgStream_ << " " << utils::basename(fileName) << ":" << line << " ";
> +	std::ostringstream ossFileInfo;
> +	ossFileInfo << utils::basename(fileName) << ":" << line;
> +	fileInfo_ = ossFileInfo.str();
>  }
>  
>  LogMessage::~LogMessage()
> @@ -515,10 +785,8 @@ LogMessage::~LogMessage()
>  
>  	msgStream_ << std::endl;
>  
> -	if (severity_ >= category_.severity()) {
> -		std::string msg(msgStream_.str());
> -		Logger::instance()->write(msg);
> -	}
> +	if (severity_ >= category_.severity())
> +		Logger::instance()->write(*this);
>  
>  	if (severity_ == LogSeverity::LogFatal)
>  		std::abort();
> @@ -534,6 +802,36 @@ LogMessage::~LogMessage()
>   * \return A reference to the log message stream
>   */
>  
> +/**
> + * \fn LogMessage::timestamp()
> + * \brief Retrieve the timestamp of the log message
> + * \return The timestamp of the message
> + */
> +
> +/**
> + * \fn LogMessage::severity()
> + * \brief Retrieve the severity of the log message
> + * \return The severity of the message
> + */
> +
> +/**
> + * \fn LogMessage::category()
> + * \brief Retrieve the category of the log message
> + * \return The category of the message
> + */
> +
> +/**
> + * \fn LogMessage::fileInfo()
> + * \brief Retrieve the file info of the log message
> + * \return The file info of the message
> + */
> +
> +/**
> + * \fn LogMessage::msg()
> + * \brief Getter for the message text of the log message

s/Getter for/Retrieve/

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

> + * \return The message text of the message, as a string
> + */
> +
>  /**
>   * \class Loggable
>   * \brief Base class to support log message extensions