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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Jul 11, 2019 at 07:24:17PM +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 v2:
> - add documentation
> - actually set the log level
> 
>  include/libcamera/logging.h   | 17 +++++++++++
>  include/libcamera/meson.build |  1 +
>  src/libcamera/log.cpp         | 54 +++++++++++++++++++++++++++++++++++
>  3 files changed, 72 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..47c5e49
> --- /dev/null
> +++ b/include/libcamera/logging.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.

2019

> + *
> + * log.h - Logging infrastructure

logging.h

> + */
> +#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..dca4936 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,57 @@ private:
>  	std::ostream *output_;
>  };
>  
> +/**
> + * \brief Set the log file
> + * \param[in] file

This is a public API so it should be properly documented. 

> + */
> +void logSetFile(const char *file)
> +{
> +	if (std::string(file).empty())
> +		return;

	if (!file[0])
		return;

But we don't usually check if mandatory parameters are null, so I would
remove this check. Or maybe we should close the log file in that case
and revert to stdout ?

> +
> +	Logger *logger = Logger::instance();
> +	std::ofstream &file_ = logger->file_;
> +	file_.close();
> +	file_.open(file);
> +	if (file_.good())
> +		logger->output_ = &file_;

Shouldn't we catch error and avoid closing the log file in that case ?
How about

	std::ofstream logFile(file);
	if (!logFile.good())
		return -EINVAL;

	logger->file_ = std::move(logFile);
	logger->output_ = &logger->file_;

	return 0;

> +}
> +
> +/**
> + * \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 should be one of the following strings:

s/should/shall/

> + * - "DEBUG"
> + * - "INFO"
> + * - "WARN"
> + * - "ERROR"
> + * - "FATAL"
> + *
> + * "*" is not a valid \a category for this function.

s/function/method/

> + */
> +void logSetLevel(const char *category, const char *level)
> +{
> +	Logger *logger = Logger::instance();
> +	std::string cat(category);
> +	std::string lev(level);
> +
> +	/* Both the category and the level must be specified. */
> +	if (cat.empty() || lev.empty())
> +		return;
> +

Same here, I think you can remove this check.

> +	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