[libcamera-devel] [PATCH] libcamera: log: Destroy LogCategory instances in a controlled way

[Hirokazu Honda]

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>