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

1653 diff mbox series

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

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@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
>   *