[libcamera-devel] [PATCH] libcamera: log: Destroy LogCategory instances in a controlled way
Hirokazu Honda
hiroh at chromium.org
Mon May 24 07:19:03 CEST 2021
Hi Laurent, thank you for the patch.
On Sun, May 23, 2021 at 9:04 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> The LogCategory instances are constructed on first use as static
> variables in accessor functions, following the Meyers singleton pattern.
> As a result, their destruction order is not guaranteed. This can cause
> issues as the global Logger object, constructed in a similar fashion, is
> accessed from the LogCategory destructor and may be destroyed first.
>
> To fix this, keep the same singleton pattern, but allocate the
> LogCategory instances dynamically. As they get registered with the
> global Logger instance, we can destroy them in the Logger destructor.
>
> This only avoids destruction order issues between LogCategory and
> Logger, and doesn't address yet the fact that LOG() calls from
> destructors of global objects may access an already destroyed Logger.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/internal/log.h | 5 ++---
> src/libcamera/log.cpp | 32 +++++++++++---------------------
> 2 files changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/include/libcamera/internal/log.h
> b/include/libcamera/internal/log.h
> index b66bf55bc57d..1032bdd93e7c 100644
> --- a/include/libcamera/internal/log.h
> +++ b/include/libcamera/internal/log.h
> @@ -29,7 +29,6 @@ class LogCategory
> {
> public:
> explicit LogCategory(const char *name);
> - ~LogCategory();
> const char *name() const { return name_; }
> LogSeverity severity() const { return severity_; }
> @@ -48,8 +47,8 @@ extern const LogCategory &_LOG_CATEGORY(name)();
> #define LOG_DEFINE_CATEGORY(name) \
> const LogCategory &_LOG_CATEGORY(name)() \
> { \
> - static LogCategory category(#name); \
> - return category; \
>
I would like to have a comment that the created category will be deleted on
Logger destruction?
With that, this change looks good to me.
Reviewed-by: Hirokazu Honda <hiroh at chormium.org>
+ static LogCategory *category = new LogCategory(#name); \
> + return *category; \
> }
>
> class LogMessage
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index dd991647b9bc..74829a56916e 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -248,6 +248,8 @@ void LogOutput::writeStream(const std::string &str)
> class Logger
> {
> public:
> + ~Logger();
> +
> static Logger *instance();
>
> void write(const LogMessage &msg);
> @@ -267,7 +269,6 @@ private:
>
> friend LogCategory;
> void registerCategory(LogCategory *category);
> - void unregisterCategory(LogCategory *category);
>
> std::unordered_set<LogCategory *> categories_;
> std::list<std::pair<std::string, LogSeverity>> levels_;
> @@ -369,6 +370,12 @@ void logSetLevel(const char *category, const char
> *level)
> Logger::instance()->logSetLevel(category, level);
> }
>
> +Logger::~Logger()
> +{
> + for (LogCategory *category : categories_)
> + delete category;
> +}
> +
> /**
> * \brief Retrieve the logger instance
> *
> @@ -665,18 +672,6 @@ void Logger::registerCategory(LogCategory *category)
}
> }
>
> -/**
> - * \brief Unregister a log category from the logger
> - * \param[in] category The log category
> - *
> - * If the \a category hasn't been registered with the logger this function
> - * performs no operation.
> - */
> -void Logger::unregisterCategory(LogCategory *category)
> -{
> - categories_.erase(category);
> -}
> -
> /**
> * \enum LogSeverity
> * Log message severity
> @@ -711,11 +706,6 @@ LogCategory::LogCategory(const char *name)
> Logger::instance()->registerCategory(this);
> }
>
> -LogCategory::~LogCategory()
> -{
> - Logger::instance()->unregisterCategory(this);
> -}
> -
> /**
> * \fn LogCategory::name()
> * \brief Retrieve the log category name
> @@ -746,12 +736,12 @@ void LogCategory::setSeverity(LogSeverity severity)
> * The default log category is named "default" and is used by the LOG()
> macro
> * when no log category is specified.
> *
> - * \return A pointer to the default log category
> + * \return A reference to the default log category
> */
> const LogCategory &LogCategory::defaultCategory()
> {
> - static const LogCategory category("default");
> - return category;
> + static const LogCategory *category = new LogCategory("default");
> + return *category;
> }
>
> /**
> --
> Regards,
>
> Laurent Pinchart
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210524/7a922515/attachment.htm>
More information about the libcamera-devel
mailing list