| Message ID | 20250130195811.1230581-9-pobrn@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Thu, Jan 30, 2025 at 07:58:57PM +0000, Barnabás Pőcze wrote: > Log categories may be added from any thread, so it is important > to synchronize access to the `Logger::categories_` list between > its two users: `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. > > To achieve that, `Logger::{find,register}Category()` are merged into > a single function: `Logger::findOrCreateCategory()`; and the mutex > from `LogCategory::create()` is moved into the `Logger` class. > > Furthermore, appropriate `MutexLocker`s are placed in > `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> The two {find,register}Category() were indeed called consecutively, so for the current usage is fine unifying the two.. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > include/libcamera/base/log.h | 2 +- > src/libcamera/base/log.cpp | 53 ++++++++++++------------------------ > 2 files changed, 19 insertions(+), 36 deletions(-) > > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h > index 1fb92603f..b32ec4516 100644 > --- a/include/libcamera/base/log.h > +++ b/include/libcamera/base/log.h > @@ -30,6 +30,7 @@ enum LogSeverity { > class LogCategory > { > public: > + explicit LogCategory(std::string_view name); > static LogCategory *create(std::string_view name); > > const std::string &name() const { return name_; } > @@ -39,7 +40,6 @@ public: > static const LogCategory &defaultCategory(); > > private: > - explicit LogCategory(std::string_view name); > > const std::string name_; > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > index 19fc5cc67..69aa16c4c 100644 > --- a/src/libcamera/base/log.cpp > +++ b/src/libcamera/base/log.cpp > @@ -402,11 +402,11 @@ private: > void parseLogFile(); > > friend LogCategory; > - void registerCategory(LogCategory *category); > - LogCategory *findCategory(std::string_view name) const; > + LogCategory *findOrCreateCategory(std::string_view name); > > static bool destroyed_; > > + Mutex mutex_; > std::vector<LogCategory *> categories_; > std::list<std::pair<std::string, LogSeverity>> levels_; > > @@ -657,6 +657,8 @@ void Logger::logSetLevel(const char *category, const char *level) > if (severity == LogInvalid) > return; > > + MutexLocker locker(mutex_); > + > for (LogCategory *c : categories_) { > if (c->name() == category) { > c->setSeverity(severity); > @@ -707,17 +709,21 @@ void Logger::parseLogFile() > } > > /** > - * \brief Register a log category with the logger > - * \param[in] category The log category > - * > - * Log categories must have unique names. It is invalid to call this function > - * if a log category with the same name already exists. > + * \brief Find an existing log category with the given name or create one > + * \param[in] name Name of the log category > + * \return The pointer to the log category found or created > */ > -void Logger::registerCategory(LogCategory *category) > +LogCategory *Logger::findOrCreateCategory(std::string_view name) > { > - categories_.push_back(category); > + MutexLocker locker(mutex_); > + > + for (LogCategory *category : categories_) { > + if (category->name() == name) > + return category; > + } > + > + LogCategory *category = categories_.emplace_back(new LogCategory(name)); > > - const std::string &name = category->name(); > for (const std::pair<std::string, LogSeverity> &level : levels_) { > bool match = true; > > @@ -737,22 +743,8 @@ void Logger::registerCategory(LogCategory *category) > break; > } > } > -} > > -/** > - * \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(std::string_view 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; > + return category; > } > > /** > @@ -790,16 +782,7 @@ LogCategory *Logger::findCategory(std::string_view name) const > */ > LogCategory *LogCategory::create(std::string_view name) > { > - static Mutex mutex_; > - MutexLocker locker(mutex_); > - LogCategory *category = Logger::instance()->findCategory(name); > - > - if (!category) { > - category = new LogCategory(name); > - Logger::instance()->registerCategory(category); > - } > - > - return category; > + return Logger::instance()->findOrCreateCategory(name); > } > > /** > -- > 2.48.1 > >
Hi Barnabás, Thank you for the patch. On Thu, Jan 30, 2025 at 07:58:57PM +0000, Barnabás Pőcze wrote: > Log categories may be added from any thread, so it is important > to synchronize access to the `Logger::categories_` list between > its two users: `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. findOrCreateCategory() is a new function, it's not a current user. You could write something along those lines: Log categories may be added from any thread, so it is important to synchronize access to the `Logger::categories_` list between its two users: category creation (by LogCategory::create(), which calls Logger::findCategory() and Logger::registerCategory()) and log level setting (by Logger::logSetLevel()). > To achieve that, `Logger::{find,register}Category()` are merged into > a single function: `Logger::findOrCreateCategory()`; and the mutex > from `LogCategory::create()` is moved into the `Logger` class. > > Furthermore, appropriate `MutexLocker`s are placed in > `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. It took me a bit of time to understand the patch, here's a proposal for an improved commit message. The LogCategory::create() function uses a mutex to serialize category creation, but Logger::logSetLevel() can access Logger::categories_ concurrently without any protection. To fix the issue, move the mutex to the Logger class, and use it to protect all accesses to the categories list. This requires moving all the logic of LogCategory::create() to a new Logger::findOrCreateCategory() function that combines both Logger::findCategory() and Logger::registerCategory() in order to make the two operations exacute atomically. > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > include/libcamera/base/log.h | 2 +- > src/libcamera/base/log.cpp | 53 ++++++++++++------------------------ > 2 files changed, 19 insertions(+), 36 deletions(-) > > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h > index 1fb92603f..b32ec4516 100644 > --- a/include/libcamera/base/log.h > +++ b/include/libcamera/base/log.h > @@ -30,6 +30,7 @@ enum LogSeverity { > class LogCategory > { > public: > + explicit LogCategory(std::string_view name); It's not very nice to make this publicly available, as it shouldn't be used anywhere else than from the create() function (now) or the findOrCreateCategory() function (with this patch applied). Wouldn't it be better to keep the constructor private and make the Logger class a friend ? > static LogCategory *create(std::string_view name); > > const std::string &name() const { return name_; } > @@ -39,7 +40,6 @@ public: > static const LogCategory &defaultCategory(); > > private: > - explicit LogCategory(std::string_view name); > > const std::string name_; > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > index 19fc5cc67..69aa16c4c 100644 > --- a/src/libcamera/base/log.cpp > +++ b/src/libcamera/base/log.cpp > @@ -402,11 +402,11 @@ private: > void parseLogFile(); > > friend LogCategory; > - void registerCategory(LogCategory *category); > - LogCategory *findCategory(std::string_view name) const; > + LogCategory *findOrCreateCategory(std::string_view name); > > static bool destroyed_; > > + Mutex mutex_; > std::vector<LogCategory *> categories_; Can we add thread safety annotations to indicate that categories_ is protected by mutex_ ? > std::list<std::pair<std::string, LogSeverity>> levels_; > > @@ -657,6 +657,8 @@ void Logger::logSetLevel(const char *category, const char *level) > if (severity == LogInvalid) > return; > > + MutexLocker locker(mutex_); > + > for (LogCategory *c : categories_) { > if (c->name() == category) { > c->setSeverity(severity); > @@ -707,17 +709,21 @@ void Logger::parseLogFile() > } > > /** > - * \brief Register a log category with the logger > - * \param[in] category The log category > - * > - * Log categories must have unique names. It is invalid to call this function > - * if a log category with the same name already exists. > + * \brief Find an existing log category with the given name or create one > + * \param[in] name Name of the log category > + * \return The pointer to the log category found or created > */ > -void Logger::registerCategory(LogCategory *category) > +LogCategory *Logger::findOrCreateCategory(std::string_view name) > { > - categories_.push_back(category); > + MutexLocker locker(mutex_); > + > + for (LogCategory *category : categories_) { > + if (category->name() == name) > + return category; > + } So nobody likes std::find_if() ? :-( I can survive its removal. > + > + LogCategory *category = categories_.emplace_back(new LogCategory(name)); > > - const std::string &name = category->name(); > for (const std::pair<std::string, LogSeverity> &level : levels_) { > bool match = true; > > @@ -737,22 +743,8 @@ void Logger::registerCategory(LogCategory *category) > break; > } > } > -} > > -/** > - * \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(std::string_view 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; > + return category; > } > > /** > @@ -790,16 +782,7 @@ LogCategory *Logger::findCategory(std::string_view name) const > */ > LogCategory *LogCategory::create(std::string_view name) > { > - static Mutex mutex_; > - MutexLocker locker(mutex_); > - LogCategory *category = Logger::instance()->findCategory(name); > - > - if (!category) { > - category = new LogCategory(name); > - Logger::instance()->registerCategory(category); > - } > - > - return category; > + return Logger::instance()->findOrCreateCategory(name); > } > > /**
Hi 2025. február 6., csütörtök 18:11 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Thu, Jan 30, 2025 at 07:58:57PM +0000, Barnabás Pőcze wrote: > > Log categories may be added from any thread, so it is important > > to synchronize access to the `Logger::categories_` list between > > its two users: `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. > > findOrCreateCategory() is a new function, it's not a current user. You > could write something along those lines: > > Log categories may be added from any thread, so it is important to > synchronize access to the `Logger::categories_` list between its two > users: category creation (by LogCategory::create(), which calls > Logger::findCategory() and Logger::registerCategory()) and log level > setting (by Logger::logSetLevel()). > > > To achieve that, `Logger::{find,register}Category()` are merged into > > a single function: `Logger::findOrCreateCategory()`; and the mutex > > from `LogCategory::create()` is moved into the `Logger` class. > > > > Furthermore, appropriate `MutexLocker`s are placed in > > `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. > > It took me a bit of time to understand the patch, here's a proposal for > an improved commit message. > > The LogCategory::create() function uses a mutex to serialize category > creation, but Logger::logSetLevel() can access Logger::categories_ > concurrently without any protection. To fix the issue, move the mutex to > the Logger class, and use it to protect all accesses to the categories > list. This requires moving all the logic of LogCategory::create() to a > new Logger::findOrCreateCategory() function that combines both > Logger::findCategory() and Logger::registerCategory() in order to make > the two operations exacute atomically. ACK > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > include/libcamera/base/log.h | 2 +- > > src/libcamera/base/log.cpp | 53 ++++++++++++------------------------ > > 2 files changed, 19 insertions(+), 36 deletions(-) > > > > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h > > index 1fb92603f..b32ec4516 100644 > > --- a/include/libcamera/base/log.h > > +++ b/include/libcamera/base/log.h > > @@ -30,6 +30,7 @@ enum LogSeverity { > > class LogCategory > > { > > public: > > + explicit LogCategory(std::string_view name); > > It's not very nice to make this publicly available, as it shouldn't be > used anywhere else than from the create() function (now) or the > findOrCreateCategory() function (with this patch applied). Wouldn't it > be better to keep the constructor private and make the Logger class a > friend ? I think this change might be here by mistake. I think it might only be needed for the last patch. (It is unfortunate that having only private constructors prevents the use of `make_{unique,shared}()`, `emplace_back()`, etc.) > > > static LogCategory *create(std::string_view name); > > > > const std::string &name() const { return name_; } > > @@ -39,7 +40,6 @@ public: > > static const LogCategory &defaultCategory(); > > > > private: > > - explicit LogCategory(std::string_view name); > > > > const std::string name_; > > > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > > index 19fc5cc67..69aa16c4c 100644 > > --- a/src/libcamera/base/log.cpp > > +++ b/src/libcamera/base/log.cpp > > @@ -402,11 +402,11 @@ private: > > void parseLogFile(); > > > > friend LogCategory; > > - void registerCategory(LogCategory *category); > > - LogCategory *findCategory(std::string_view name) const; > > + LogCategory *findOrCreateCategory(std::string_view name); > > > > static bool destroyed_; > > > > + Mutex mutex_; > > std::vector<LogCategory *> categories_; > > Can we add thread safety annotations to indicate that categories_ is > protected by mutex_ ? I think so. > > > std::list<std::pair<std::string, LogSeverity>> levels_; > > > > @@ -657,6 +657,8 @@ void Logger::logSetLevel(const char *category, const char *level) > > if (severity == LogInvalid) > > return; > > > > + MutexLocker locker(mutex_); > > + > > for (LogCategory *c : categories_) { > > if (c->name() == category) { > > c->setSeverity(severity); > > @@ -707,17 +709,21 @@ void Logger::parseLogFile() > > } > > > > /** > > - * \brief Register a log category with the logger > > - * \param[in] category The log category > > - * > > - * Log categories must have unique names. It is invalid to call this function > > - * if a log category with the same name already exists. > > + * \brief Find an existing log category with the given name or create one > > + * \param[in] name Name of the log category > > + * \return The pointer to the log category found or created > > */ > > -void Logger::registerCategory(LogCategory *category) > > +LogCategory *Logger::findOrCreateCategory(std::string_view name) > > { > > - categories_.push_back(category); > > + MutexLocker locker(mutex_); > > + > > + for (LogCategory *category : categories_) { > > + if (category->name() == name) > > + return category; > > + } > > So nobody likes std::find_if() ? :-( I can survive its removal. > > > + > > + LogCategory *category = categories_.emplace_back(new LogCategory(name)); > > > > - const std::string &name = category->name(); > > for (const std::pair<std::string, LogSeverity> &level : levels_) { > > bool match = true; > > > > @@ -737,22 +743,8 @@ void Logger::registerCategory(LogCategory *category) > > break; > > } > > } > > -} > > > > -/** > > - * \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(std::string_view 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; > > + return category; > > } > > > > /** > > @@ -790,16 +782,7 @@ LogCategory *Logger::findCategory(std::string_view name) const > > */ > > LogCategory *LogCategory::create(std::string_view name) > > { > > - static Mutex mutex_; > > - MutexLocker locker(mutex_); > > - LogCategory *category = Logger::instance()->findCategory(name); > > - > > - if (!category) { > > - category = new LogCategory(name); > > - Logger::instance()->registerCategory(category); > > - } > > - > > - return category; > > + return Logger::instance()->findOrCreateCategory(name); > > } > > > > /** > > -- > Regards, > > Laurent Pinchart >
diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h index 1fb92603f..b32ec4516 100644 --- a/include/libcamera/base/log.h +++ b/include/libcamera/base/log.h @@ -30,6 +30,7 @@ enum LogSeverity { class LogCategory { public: + explicit LogCategory(std::string_view name); static LogCategory *create(std::string_view name); const std::string &name() const { return name_; } @@ -39,7 +40,6 @@ public: static const LogCategory &defaultCategory(); private: - explicit LogCategory(std::string_view name); const std::string name_; diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp index 19fc5cc67..69aa16c4c 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -402,11 +402,11 @@ private: void parseLogFile(); friend LogCategory; - void registerCategory(LogCategory *category); - LogCategory *findCategory(std::string_view name) const; + LogCategory *findOrCreateCategory(std::string_view name); static bool destroyed_; + Mutex mutex_; std::vector<LogCategory *> categories_; std::list<std::pair<std::string, LogSeverity>> levels_; @@ -657,6 +657,8 @@ void Logger::logSetLevel(const char *category, const char *level) if (severity == LogInvalid) return; + MutexLocker locker(mutex_); + for (LogCategory *c : categories_) { if (c->name() == category) { c->setSeverity(severity); @@ -707,17 +709,21 @@ void Logger::parseLogFile() } /** - * \brief Register a log category with the logger - * \param[in] category The log category - * - * Log categories must have unique names. It is invalid to call this function - * if a log category with the same name already exists. + * \brief Find an existing log category with the given name or create one + * \param[in] name Name of the log category + * \return The pointer to the log category found or created */ -void Logger::registerCategory(LogCategory *category) +LogCategory *Logger::findOrCreateCategory(std::string_view name) { - categories_.push_back(category); + MutexLocker locker(mutex_); + + for (LogCategory *category : categories_) { + if (category->name() == name) + return category; + } + + LogCategory *category = categories_.emplace_back(new LogCategory(name)); - const std::string &name = category->name(); for (const std::pair<std::string, LogSeverity> &level : levels_) { bool match = true; @@ -737,22 +743,8 @@ void Logger::registerCategory(LogCategory *category) break; } } -} -/** - * \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(std::string_view 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; + return category; } /** @@ -790,16 +782,7 @@ LogCategory *Logger::findCategory(std::string_view name) const */ LogCategory *LogCategory::create(std::string_view name) { - static Mutex mutex_; - MutexLocker locker(mutex_); - LogCategory *category = Logger::instance()->findCategory(name); - - if (!category) { - category = new LogCategory(name); - Logger::instance()->registerCategory(category); - } - - return category; + return Logger::instance()->findOrCreateCategory(name); } /**
Log categories may be added from any thread, so it is important to synchronize access to the `Logger::categories_` list between its two users: `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. To achieve that, `Logger::{find,register}Category()` are merged into a single function: `Logger::findOrCreateCategory()`; and the mutex from `LogCategory::create()` is moved into the `Logger` class. Furthermore, appropriate `MutexLocker`s are placed in `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- include/libcamera/base/log.h | 2 +- src/libcamera/base/log.cpp | 53 ++++++++++++------------------------ 2 files changed, 19 insertions(+), 36 deletions(-)