[libcamera-devel] [PATCH v2 2/2] libcamera: base: log: Fix LogCategory creation issues
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Oct 19 14:59:29 CEST 2022
Quoting Laurent Pinchart via libcamera-devel (2022-08-29 15:17:17)
> Hi Tomi,
>
> Thank you for the patch.
>
> On Mon, Aug 29, 2022 at 11:44:45AM +0300, Tomi Valkeinen wrote:
> > Each declaration of a LogCategory will create a new LogCategory, and
> > will be stored in an unordered_set Logger::categories_. This means that
> > when a plugin .so is unloaded and loaded, as happens when destructing
> > and creating a CamereManager, we'll get duplicate categories.
> >
> > The Logger::registerCategory docs say "Log categories must have unique
> > names. If a category with the same name already exists this function
> > performs no operation.". The code does not comply with this.
> >
> > We solve the issue with two changes:
> >
> > Change the unordered_set to a vector for simplicity, as there's no need
> > for an unordered_set.
> >
> > Instead of using the LogCategory constructor to create new categories in
> > _LOG_CATEGORY() macro, use a factory method. The factory method will
> > return either an existing LogCategory if one exists with the given name,
> > or a newly created one.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> > ---
> > include/libcamera/base/log.h | 6 +++--
> > src/libcamera/base/log.cpp | 49 +++++++++++++++++++++++++++++++-----
> > 2 files changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> > index 8b462767..dcaacbe0 100644
> > --- a/include/libcamera/base/log.h
> > +++ b/include/libcamera/base/log.h
> > @@ -29,7 +29,7 @@ enum LogSeverity {
> > class LogCategory
> > {
> > public:
> > - explicit LogCategory(const char *name);
> > + static LogCategory *create(const char *name);
> >
> > const std::string &name() const { return name_; }
> > LogSeverity severity() const { return severity_; }
> > @@ -38,6 +38,8 @@ public:
> > static const LogCategory &defaultCategory();
> >
> > private:
> > + explicit LogCategory(const char *name);
> > +
> > const std::string name_;
> > LogSeverity severity_;
> > };
> > @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)();
> > const LogCategory &_LOG_CATEGORY(name)() \
> > { \
> > /* The instance will be deleted by the Logger destructor. */ \
> > - static LogCategory *category = new LogCategory(#name); \
> > + static LogCategory *category = LogCategory::create(#name); \
> > return *category; \
> > }
> >
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index a4a5b452..8bf1beb9 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -314,10 +314,11 @@ private:
> >
> > friend LogCategory;
> > void registerCategory(LogCategory *category);
> > + LogCategory *findCategory(const char *name) const;
> >
> > static bool destroyed_;
> >
> > - std::unordered_set<LogCategory *> categories_;
> > + std::vector<LogCategory *> categories_;
> > std::list<std::pair<std::string, LogSeverity>> levels_;
> >
> > std::shared_ptr<LogOutput> output_;
> > @@ -707,12 +708,12 @@ LogSeverity Logger::parseLogLevel(const std::string &level)
> > * \brief Register a log category with the logger
> > * \param[in] category The log category
> > *
> > - * Log categories must have unique names. If a category with the same name
> > - * already exists this function performs no operation.
> > + * Log categories must have unique names. It is invalid to call this function
> > + * if a log category with the same name already exists.
> > */
> > void Logger::registerCategory(LogCategory *category)
> > {
> > - categories_.insert(category);
> > + categories_.push_back(category);
> >
> > const std::string &name = category->name();
> > for (const std::pair<std::string, LogSeverity> &level : levels_) {
> > @@ -736,6 +737,22 @@ void Logger::registerCategory(LogCategory *category)
> > }
> > }
> >
> > +/**
> > + * \brief Find an existing log category with the given name
> > + * \param[in] name Name of the log category
> > + * \return The pointer to the found log category or nullptr if not found.
>
> s/.$//
>
> I'll fix when applying.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > + */
> > +LogCategory *Logger::findCategory(const char *name) const
> > +{
> > + if (auto it = std::find_if(categories_.begin(), categories_.end(),
> > + [name](auto c) { return c->name() == name; });
> > + it != categories_.end()) {
> > + return *it;
> > + }
> > +
> > + return nullptr;
> > +}
> > +
> > /**
> > * \enum LogSeverity
> > * Log message severity
> > @@ -760,6 +777,27 @@ void Logger::registerCategory(LogCategory *category)
> > * and is used to control the log level per group.
> > */
> >
> > +/**
> > + * \brief Create a new LogCategory or return an existing one
> > + * \param[in] name Name of the log category
> > + *
> > + * Create and return a new LogCategory with the given name if such a category
> > + * does not yet exist, or return the existing one.
> > + *
> > + * \return The pointer to the LogCategory
> > + */
> > +LogCategory *LogCategory::create(const char *name)
> > +{
> > + LogCategory *category = Logger::instance()->findCategory(name);
> > +
> > + if (!category) {
> > + category = new LogCategory(name);
> > + Logger::instance()->registerCategory(category);
> > + }
> > +
> > + return category;
> > +}
> > +
> > /**
> > * \brief Construct a log category
> > * \param[in] name The category name
> > @@ -767,7 +805,6 @@ void Logger::registerCategory(LogCategory *category)
> > LogCategory::LogCategory(const char *name)
> > : name_(name), severity_(LogSeverity::LogInfo)
> > {
> > - Logger::instance()->registerCategory(this);
> > }
> >
> > /**
> > @@ -804,7 +841,7 @@ void LogCategory::setSeverity(LogSeverity severity)
> > */
> > const LogCategory &LogCategory::defaultCategory()
> > {
> > - static const LogCategory *category = new LogCategory("default");
> > + static const LogCategory *category = LogCategory::create("default");
> > return *category;
> > }
> >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list