[libcamera-devel,v2,2/2] libcamera: base: log: Fix LogCategory creation issues
diff mbox series

Message ID 20220829084445.61585-3-tomi.valkeinen@ideasonboard.com
State Accepted
Headers show
Series
  • LogCategory fixing
Related show

Commit Message

Tomi Valkeinen Aug. 29, 2022, 8:44 a.m. UTC 
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@ideasonboard.com>
---
 include/libcamera/base/log.h |  6 +++--
 src/libcamera/base/log.cpp   | 49 +++++++++++++++++++++++++++++++-----
 2 files changed, 47 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart Aug. 29, 2022, 2:17 p.m. UTC | #1 
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@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@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;
>  }
>
Kieran Bingham Oct. 19, 2022, 12:59 p.m. UTC | #2 
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@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@ideasonboard.com>


Reviewed-by: Kieran Bingham <kieran.bingham@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

Patch
diff mbox series 

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.
+ */
+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;
 }