Skip to content

refactor: Handle signals by g_unix_signal_add#206

Merged
deepin-bot[bot] merged 2 commits intolinuxdeepin:develop/snipefrom
wangrong1069:pr0310
Mar 10, 2026
Merged

refactor: Handle signals by g_unix_signal_add#206
deepin-bot[bot] merged 2 commits intolinuxdeepin:develop/snipefrom
wangrong1069:pr0310

Conversation

@wangrong1069
Copy link
Copy Markdown
Contributor

@wangrong1069 wangrong1069 commented Mar 10, 2026

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @wangrong1069, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

- Add AGENTS.md with build, test, lint commands and coding guidelines
- Update .gitignore to ignore kernel module build artifacts
- Remove systemd service start limits from daemon service

Bug: https://pms.uniontech.com/bug-view-349109.html
Remove the custom signal handler implementation and use GLib's
`g_unix_signal_add()` for SIGINT and SIGTERM handling. It converts
insecure signals into secure Loop events, thus achieving secure signal
processing.
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这份代码变更主要涉及项目文档更新、许可证信息修正、服务配置调整以及信号处理机制的重构。以下是对这些变更的详细审查意见:

1. 总体评价

本次代码变更整体质量较高,主要目的是规范化项目结构、更新许可证年份范围,并将自定义的信号处理机制替换为更标准的 GLib 信号处理方式。这有助于提高代码的可维护性和安全性。

2. 详细审查意见

A. .gitignore 变更

变更内容:添加了内核模块构建过程中的临时文件忽略规则。

+# kernelmod
+src/kernelmod/.Module.symvers.cmd
+src/kernelmod/.modules.order.cmd
+src/kernelmod/.vfs_monitor.mod.cmd

审查意见

  • 正确性:正确添加了内核模块编译生成的临时命令文件,避免将构建中间产物提交到版本控制。
  • 建议:这些文件是 make modules 过程中生成的,忽略它们是正确的做法。

B. AGENTS.md 新增文档

变更内容:新增了一份关于构建、测试和代码风格的指南文档。
审查意见

  • 语法逻辑:文档结构清晰,Markdown 格式规范。
  • 代码质量
    • 明确了不同模块的 C++ 标准(C++17, C++20)和 C 标准(C99)。
    • 规定了命名规范(snake_case, CamelCase)和格式(K&R 风格,4空格缩进)。
    • 明确了错误处理策略(0 成功,非 0 失败)。
  • 代码安全
    • 强调了检查 malloc/calloc 返回值,这是防止空指针解引用的关键。
    • 建议使用 strncpysnprintf 替代不安全的字符串函数,有助于防止缓冲区溢出。
  • 建议
    • Test Commands 部分标记为 Not support。建议补充单元测试计划,或者说明为什么暂不支持自动化测试(例如内核模块难以模拟)。
    • 文档中提到 "Aim for >80% code coverage",但测试部分为空,存在矛盾。建议明确测试覆盖率的目标和工具。

C. REUSE.toml 变更

变更内容:将新文件 AGENTS.md 添加到 CC0-1.0 许可证列表中。
审查意见

  • 正确性:符合 REUSE 规范,确保文档文件有明确的许可证声明。
  • 建议:无。

D. deepin-anything-daemon.service 变更

变更内容:移除了 StartLimitIntervalSecStartLimitBurst 配置。

-# The time window is 3600 seconds (60 minutes)
-StartLimitIntervalSec=3600
-# A maximum of 2 starts are allowed within this window.
-StartLimitBurst=2

审查意见

  • 代码安全/稳定性
    • 风险:移除启动限制可能导致服务崩溃时无限重启,引发"重启风暴",消耗系统资源。
    • 解释:原配置限制了在 60 分钟内最多重启 2 次。移除后,如果服务存在启动即崩溃的 Bug,systemd 将会无限制地尝试重启它。
  • 建议
    • 强烈建议:保留或调整这些限制,而不是完全移除。例如,可以设置 StartLimitIntervalSec=60StartLimitBurst=5,允许更频繁的重启尝试但仍有上限。
    • 如果移除是有意为之(例如依赖上层监控),请在 Commit Message 中说明原因。

E. src/daemon/include/anything.hpp 变更

变更内容:更新了许可证年份,并移除了 signal_handler.h 的引用。
审查意见

  • 正确性:许可证年份更新为 2024-2026 是合理的。移除不再使用的头文件是正确的。
  • 建议:无。

F. src/daemon/src/main.cpp 变更

变更内容:使用 GLib 的 g_unix_signal_add 替换了自定义的 set_signal_handler

+// 收到信号后调用, 运行在 Qt 事件循环中
+gboolean on_sigint_sigterm(gpointer user_data) {
+    spdlog::info("Interrupt signal ({}) received, quit", (const char *) user_data);
+    set_app_restart(false);
+    qApp->quit();
+    return TRUE;
+}
...
+    guint sigint_id = g_unix_signal_add(SIGINT, on_sigint_sigterm, (gpointer)"SIGINT");
+    guint sigterm_id = g_unix_signal_add(SIGTERM, on_sigint_sigterm, (gpointer)"SIGTERM");
...
+    if (sigint_id > 0)
+        g_source_remove(sigint_id);

审查意见

  • 代码质量
    • 改进:使用 g_unix_signal_add 是处理信号的最佳实践,特别是在涉及 GLib 主循环(如 Qt 事件循环)时。它将信号处理安全地分发到主线程,避免了自定义信号处理中常见的竞态条件和重入问题(如在信号处理函数中调用非异步信号安全函数 spdlog::infoqApp->quit)。
    • 改进:代码中明确注释了"运行在 Qt 事件循环中",这解释了为什么可以安全调用 Qt 函数。
  • 代码安全
    • 改进:之前的实现直接在信号处理函数(default_signal_handler)中调用 std::function,这是不安全的,因为 std::function 的调用涉及内存分配,不是异步信号安全的。新的实现通过 GLib 机制避免了这个问题。
    • 细节on_sigint_sigterm 中的 (const char *) user_data 强转虽然在这里是安全的(因为传入的是字符串字面量),但类型转换需谨慎。
  • 代码性能
    • GLib 的信号处理机制略有开销,但对于 SIGINT/SIGTERM 这种低频事件,性能影响可以忽略不计。
  • 逻辑问题
    • main 函数末尾,app.exec() 返回后才清理信号源。这是正确的,因为程序即将退出。
    • g_source_remove 的检查 if (sigint_id > 0) 是好的防御性编程,虽然 g_unix_signal_add 失败通常返回 0,但在多线程环境下这种检查是必要的。

G. 删除文件

变更内容:删除了 signal_handler.hsignal_handler.cpp
审查意见

  • 正确性:既然功能已被 g_unix_signal_add 替代,删除旧代码是正确的,减少了维护负担。

3. 总结与建议

  1. 关键风险deepin-anything-daemon.service 中移除服务重启限制是一个潜在的系统稳定性风险,建议恢复该配置或提供充分的解释。
  2. 代码规范AGENTS.md 文档非常有价值,建议后续开发严格遵循其中的规范,特别是 C++ 和 C 混合编程时的边界处理。
  3. 信号处理:信号处理机制的改进非常到位,从自定义的不安全实现迁移到了成熟的 GLib 机制,提升了程序的健壮性。
  4. 注释main.cpp 中新增的中文注释清晰易懂,有助于维护。

总体而言,除了服务配置文件中的重启限制移除需要再次确认外,其他变更都是积极且高质量的改进。

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lzwind, wangrong1069

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

@wangrong1069
Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot deepin-bot Bot merged commit eb216e1 into linuxdeepin:develop/snipe Mar 10, 2026
19 checks passed
@wangrong1069 wangrong1069 deleted the pr0310 branch March 10, 2026 13:43
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