Skip to content

fix: make power-snoop internal symbols static for test linking - #103

Open
yuKing123-king wants to merge 1 commit into
DKapture:mainfrom
yuKing123-king:fix/power-snoop-static-linkage
Open

yuKing123-king wants to merge 1 commit into
DKapture:mainfrom
yuKing123-king:fix/power-snoop-static-linkage

Conversation

@yuKing123-king

Copy link
Copy Markdown
Contributor

为避免 power-snoop 在单测聚合链接时与其他 observe 工具发生同名全局符号冲突,将仅在本文件使用的运行时状态和辅助函数收敛为内部链接,不改变工具功能,只修复 make test 阶段的 multiple definition 问题。

Signed-off-by: Wang Yu <wangyu6@uniontech.com>
@dkapture-ci-bot

Copy link
Copy Markdown
Contributor

你好,这是对 libdkapture#103 fix: make power-snoop internal symbols static for test linki 的评审。

本轮为 DKapture 两仓(libdkapture / dkapture-bpf)全部以 fix 开头 open PR 的批量评审,共 17 个;汇总表如下,本 PR 加粗。逐项详评见分隔线下方。

PR 标题 作者 规模 结论 主要风险
#149 fix(net-traffic): initialize rules before loadin yuKing123-king +19/-9 可合并 规则安装顺序与其它工具约定不一致;本仓改动无正确性缺陷
#147 fix(net-filter): abort rule loading on invalid c yuKing123-king +9/-3 需修改 失败时 clear_rules() 波及 add_rule() 装入的规则;空白注释行导致整个加载失败
#141 fix(dkapture): honor parsed pid in read(vector<p JoeSergen +23/-2 需修改 unsafe_find 单记录语义使 /proc//fd 只回调一次;返回值语义漂移
#140 fix(dkapture): return bytes read, not remaining JoeSergen +10/-4 可合并 无阻塞风险;与文档契约对齐,仓内无调用方依赖旧语义
#114 fix: replace manual destructor calls in construc JoeSergen +28/-7 需修改 捆绑了与 open PR #112 逐字节相同的 fs_watch 修复,跨 PR 重复
#125 fix(lsof): start ringbuf consumer before iterato yuKing123-king +17/-3 需修改 线程启动后错误路径仍 goto err_out → 释放运行中 rb(UAF)+ 线程泄漏
#112 fix: fs_watch calls trace_file_init instead of m JoeSergen +1/-1 可合并 无;仅修一处复制粘贴错误
#103 fix: make power-snoop internal symbols static fo yuKing123-king +3/-2 可合并 改动无害;但 PR 描述的 multiple definition 在当前构建配置下无法复现
#119 fix(syscall-stat): stop skipping syscall key 0 d yuKing123-king +18/-6 可合并 循环终止四条路径已逐一核实;缩进与提交拆分小问题
#115 fix(syscall-stat): improve builtin flow yuKing123-king +204/-41 需修改 三处 bpf_get_map_fd 错误路径未设 ret,最终 return ret 误报成功
#117 fix(trace-signal): validate invalid command line yuKing123-king +190/-29 需修改 BUILTIN 测试入口未接入 test/Makefile,不可达;register_signal 残留
#102 fix(trace-exec): reject invalid command line arg yuKing123-king +135/-22 需修改 BUILTIN 入口同样未接入构建;-h 退出码 0→1 属未说明的行为变更
#97 fix(so): reuse pinned dkapture bpf objects corre yuKing123-king +1/-1 需修改 test mock 仍按 map- 前缀命名 pin,合并后 gtest FindMap 用例失败
#35 fix(pagefault): add max_entries and value_size f yuKing123-king +7/-3 可合并 修复真实,但已被 main 上等效修复 1e90486 取代,建议确认后关闭
#32 fix(peek-fd): correct args field name from mvlen yuKing123-king +1/-1 可合并 无功能风险;标题/描述与实际改动方向不符
#30 fix(trace-signal): avoid inflight event key coll yuKing123-king +36/-22 需修改 sys_exit_kill 的 !rule 早退路径仍泄漏 inflight 条目
#29 fix(syscall-stat): replace exec fexit with kprob yuKing123-king +93/-13 阻塞 exec 路径从 struct filename* 本身读字符串,-f 过滤将完全失效

本 PR 评审详情

作者: yuKing123-king | 规模: +3/-2 | 文件: 1
结论: 可合并
主要风险: 改动本身无害;但 PR 描述所称的 make test 阶段 multiple definition 在当前构建配置下无法复现,描述可能过时。

总体结论: 全仓 grep 核实:power-snoop.cpp 中仅 envringbuffer_worker 具有外部链接(其余全局 rb/obj/exiting/rb_thread/global_stats 等在 observe/power-snoop.cpp:20-25,62 均已 static),且无任何其它 TU 声明或引用这两个符号(irqsnoop.cpp:25,186 与 thermal-snoop.cpp:697 中的同名符号是各自 TU 的独立定义,与 power-snoop 无耦合)。static 化边界准确,power_snoop_init/power_snoop_deinit(:522,544)作为 BUILTIN 入口正确保持外部链接。但核实链接配置发现:power-snoop.o 既不在 test/Makefile 的链接列表(在 base b369fe7 与当前 main 均不含,git show 核实),也不在 libdkapture.so(so/Makefile:9-14 仅含 trace-file/kmemleak/lsock/mountsnoop/irqsnoop),本 diff 单独无法达成"修复 make test multiple definition"的目标——除非配合未包含在 diff 中的其它改动。

主要问题: 无(改动正确、无害,不阻塞)
次要建议:

  • observe/power-snoop.cpp:45 (新) — 新增 }; 行尾带一个空格(patch 引入的 trailing whitespace),建议去除。
  • observe/irqsnoop.cpp:25,186 — 若目标是测试聚合链接卫生,irqsnoop(在 test/Makefile 链接列表中)同样有外部链接的 struct env/ringbuffer_worker,后续 PR 可一并收敛。
  • observe/power-snoop.cpp:46,497 — 与 fix(lsof): start ringbuf consumer before iterator scan #125 同作者的提交风格一致:commit message 缺根因正文,建议补一句说明冲突对象。

亮点:

  • 改动最小且边界准确,未触碰需保持外部链接的入口符号。

commit message: 标题符合 fix: 规范但无 scope、无正文,弱于仓库近期 fix(scope): + 根因正文的惯例。

已有讨论: 无

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.

2 participants