[libcamera-devel] [PATCH v3 1/2] libcamera: logging: add logging API for applications

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jul 12, 2019 at 03:05:24AM +0900, Paul Elder wrote:
> Currently the log file and the log level can only be set via environment
> variables, but applications may also want to set the log file and the
> log level at run time. Provide an API for this.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v3:
> - make setLogFile revert to stderr on empty arg
> - make both setLogFile and setLogLevel ignore empty args (not check for
>   error)
> - better docs for setLogFile
> 
> Changes in v2:
> - add documentation
> - actually set the log level
> 
>  include/libcamera/logging.h   | 17 ++++++++++
>  include/libcamera/meson.build |  1 +
>  src/libcamera/log.cpp         | 60 +++++++++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+)
>  create mode 100644 include/libcamera/logging.h
> 
> diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h
> new file mode 100644
> index 0000000..c8a048e
> --- /dev/null
> +++ b/include/libcamera/logging.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * logging.h - Logging infrastructure
> + */
> +#ifndef __LIBCAMERA_LOGGING_H__
> +#define __LIBCAMERA_LOGGING_H__
> +
> +namespace libcamera {
> +
> +void logSetFile(const char *file);
> +void logSetLevel(const char *category, const char *level);
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_LOGGING_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 972513f..920eb5f 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -9,6 +9,7 @@ libcamera_api = files([
>      'geometry.h',
>      'ipa/ipa_interface.h',
>      'ipa/ipa_module_info.h',
> +    'logging.h',
>      'object.h',
>      'request.h',
>      'signal.h',
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 0ba276e..11bac80 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -69,6 +69,9 @@ private:
>  	void parseLogLevels();
>  	static LogSeverity parseLogLevel(const std::string &level);
>  
> +	friend void logSetFile(const char *file);
> +	friend void logSetLevel(const char *category, const char *level);
> +
>  	friend LogCategory;
>  	void registerCategory(LogCategory *category);
>  	void unregisterCategory(LogCategory *category);
> @@ -80,6 +83,63 @@ private:
>  	std::ostream *output_;
>  };
>  
> +/**
> + * \brief Set the log file
> + * \param[in] file Log file

"Full path to the log file"

> + *
> + * This functions sets the logging output file to \a file.
> + * If \a file is an empty string, then the output file will be set to stderr.

 * 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.
 *
 * 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.
 *
 * \return Zero on success, or a negative error code otherwise.

> + */
> +void logSetFile(const char *file)
> +{
> +	Logger *logger = Logger::instance();
> +
> +	if (!file[0]) {

	if (!file) {

(I was tired when reviewing the previous version...)

> +		logger->file_.close();
> +		logger->output_ = &std::cerr;

Swap those two lines to avoid races (we'll have to implement locking,
but that's for later).

> +		return;
> +	}
> +
> +	std::ofstream logFile(file);
> +	if (!logFile.good())
> +		return;

		return -EINVAL;

> +
> +	if (logger->output_ != &std::cerr)
> +		logger->file_.close();
> +	logger->file_ = std::move(logFile);
> +	logger->output_ = &logger->file_;
> +}
> +
> +/**
> + * \brief Set the log level
> + * \param[in] category Logging category
> + * \param[in] level Log level
> + *
> + * This function sets the log level of \a category to \a level.
> + * \a level shall be one of the following strings:
> + * - "DEBUG"
> + * - "INFO"
> + * - "WARN"
> + * - "ERROR"
> + * - "FATAL"
> + *
> + * "*" is not a valid \a category for this function.
> + */
> +void logSetLevel(const char *category, const char *level)
> +{
> +	Logger *logger = Logger::instance();
> +	std::string cat(category);

This is unused.

> +	std::string lev(level);

This is used in a single location, you can write
Logger::parseLogLevel(level) and remove lev.

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

> +
> +	LogSeverity severity = Logger::parseLogLevel(lev);
> +	if (severity == LogInvalid)
> +		return;
> +
> +	for (LogCategory *c : logger->categories_)
> +		if (!strcmp(c->name(), category))
> +			c->setSeverity(severity);
> +}
> +
>  /**
>   * \brief Retrieve the logger instance
>   *

-- 
Regards,

Laurent Pinchart