Skip to content

feat: add appender unregistration methods#30

Merged
mhduiy merged 1 commit intolinuxdeepin:masterfrom
mhduiy:log
Feb 27, 2026
Merged

feat: add appender unregistration methods#30
mhduiy merged 1 commit intolinuxdeepin:masterfrom
mhduiy:log

Conversation

@mhduiy
Copy link
Contributor

@mhduiy mhduiy commented Feb 25, 2026

Added unregisterAppender and unregisterCategoryAppender methods to Logger class to allow proper cleanup of appender registrations. These methods remove appenders from the logger's internal lists and transfer ownership back to the caller, who becomes responsible for managing the appender's lifetime and preventing memory leaks. This is necessary for dynamic appender management scenarios where appenders need to be removed during runtime, such as when reconfiguring logging systems or shutting down components that use dedicated appenders.

feat: 添加日志输出器注销方法

为 Logger 类新增 unregisterAppender 和 unregisterCategoryAppender 方法, 以支持正确清理已注册的日志输出器。这些方法将输出器从日志记录器的内部列表
中移除,并将所有权交还给调用方,由调用方负责管理输出器的生命周期并防止内
存泄漏。这对于需要动态管理输出器的场景是必要的,例如在运行时重新配置日志
系统或关闭使用专用输出器的组件时。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhduiy

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Added unregisterAppender and unregisterCategoryAppender methods to
Logger class to allow proper cleanup of appender registrations. These
methods remove appenders from the logger's internal lists and transfer
ownership back to the caller, who becomes responsible for managing the
appender's lifetime and preventing memory leaks. This is necessary for
dynamic appender management scenarios where appenders need to be removed
during runtime, such as when reconfiguring logging systems or shutting
down components that use dedicated appenders.

feat: 添加日志输出器注销方法

为 Logger 类新增 unregisterAppender 和 unregisterCategoryAppender 方法,
以支持正确清理已注册的日志输出器。这些方法将输出器从日志记录器的内部列表
中移除,并将所有权交还给调用方,由调用方负责管理输出器的生命周期并防止内
存泄漏。这对于需要动态管理输出器的场景是必要的,例如在运行时重新配置日志
系统或关闭使用专用输出器的组件时。
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码主要包含两部分修改:一是更新了许可证声明,从传统的版权声明格式改为更现代的 SPDX 标识符;二是为 Logger 类添加了注销追加器(Appender)的功能。以下是对这两部分的详细审查和改进建议:

1. 许可证声明更新 (SPDX 标识符)

审查意见:

  • 优点
    • 标准化:将许可证信息更新为 SPDX-License-IdentifierSPDX-FileCopyrightText 是非常好的做法。这符合 REUSE 规范,有助于自动化工具进行许可证合规性检查。
    • 一致性:所有源文件和头文件都统一添加了这些标识符,保持了代码库的一致性。
    • 清晰度:保留了原有的完整许可证文本块(如 LGPL-2.1 的全文),这对于法律合规性很重要,同时也方便人类阅读。
  • 潜在问题
    • 年份错误:在 include/Logger.hsrc/Logger.cppinclude/RollingFileAppender.h 等文件中,新增的 UnionTech 版权声明年份写的是 2026。除非这是为了未来的发布准备的,否则这看起来像是一个笔误(可能是想写 20232024)。错误的年份可能会引起法律上的混淆。

改进建议:

  • 修正年份:请核实所有新增的 SPDX-FileCopyrightText 行中的年份。如果是当前修改,应使用当前年份(如 2023 或 2024)。例如:
    // SPDX-FileCopyrightText: 2023 UnionTech Software Technology Co., Ltd.
  • dep5 文件同步.reuse/dep5 文件被修改,删除了通用的 src/* include/* 条目。这表明现在依赖文件头部的 SPDX 标识符来声明版权。请确保 dep5 文件中的剩余条目与文件头部的声明完全一致,避免冲突。

2. 新增功能:注销 Appender (unregisterAppenderunregisterCategoryAppender)

审查意见:

  • 逻辑正确性
    • unregisterAppender 使用了 removeOne,这会移除列表中第一个匹配的指针。这是合理的,因为同一个指针实例不应该被多次注册。
    • unregisterCategoryAppender 使用了 QMultiMap::remove,这会移除所有匹配的键值对。这也是合理的,因为同一个 appender 可能被错误地多次注册到同一个 category。
  • 线程安全
    • 两个函数都使用了 QMutexLocker locker(&d->loggerMutex);,这保证了操作的原子性,防止在多线程环境下发生竞态条件。这是非常好的做法。
  • 内存管理
    • 注释中明确指出:"The caller becomes responsible for managing the appender's lifetime" 和 "The caller must delete it manually to prevent memory leaks."。这一点非常关键,因为 Logger 之前可能拥有这些对象的所有权(并在析构时删除它们)。现在将所有权转交给调用者,必须确保调用者知晓这一点。
    • 潜在风险:如果调用者忘记删除 appender,就会导致内存泄漏。此外,如果调用者在 appender 仍在被其他线程使用时删除它,可能会导致悬垂指针和崩溃。

改进建议:

  • 返回值
    • 建议让这两个函数返回一个 bool 值,表示是否成功找到并移除了 appender。这对于调用者进行错误处理或调试非常有帮助。
    • 例如:
      bool Logger::unregisterAppender(AbstractAppender *appender)
      {
          Q_D(Logger);
          QMutexLocker locker(&d->loggerMutex);
          return d->appenders.removeOne(appender);
      }
  • 空指针检查
    • 虽然传入 nullptrremoveOneremove 通常是安全的(它们不会崩溃),但在逻辑上注册或注销一个空指针通常是一个错误。建议添加断言或检查:
      void Logger::unregisterAppender(AbstractAppender *appender)
      {
          if (!appender) {
              return; // 或者 qWarning() << "Cannot unregister null appender";
          }
          Q_D(Logger);
          QMutexLocker locker(&d->loggerMutex);
          d->appenders.removeOne(appender);
      }
  • 文档增强
    • 在头文件的注释中,除了说明调用者需要手动删除外,还可以增加关于线程安全的警告。例如:
      /*!
       * @brief Unregisters the appender from the logger.
       * ...
       * @warning Ensure that the appender is not being used by other threads
       *          before deleting it. This function is thread-safe.
       */
  • 析构函数检查
    • 请检查 Logger 的析构函数。如果 Logger 之前负责删除 appenders,现在修改逻辑后,析构函数应该不再删除它们,或者只删除那些仍然注册的 appenders。如果析构函数仍然尝试删除已经被注销并由调用者删除的 appender,会导致双重释放(Double Free)错误。

总结

这段代码整体质量很高,特别是在许可证合规性和线程安全性方面做得很好。主要的改进点在于:

  1. 修正版权年份的笔误。
  2. unregister 函数增加返回值和空指针检查,提高健壮性。
  3. 确保析构函数与新的所有权语义保持一致,防止内存管理错误。

@mhduiy mhduiy merged commit 3fe91a1 into linuxdeepin:master Feb 27, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants