Skip to content

refactor: fix compiler warnings across runtime and utils#1665

Merged
reddevillg merged 1 commit into
OpenAtom-Linyaps:masterfrom
ComixHe:master
May 19, 2026
Merged

refactor: fix compiler warnings across runtime and utils#1665
reddevillg merged 1 commit into
OpenAtom-Linyaps:masterfrom
ComixHe:master

Conversation

@ComixHe

@ComixHe ComixHe commented May 18, 2026

Copy link
Copy Markdown
Collaborator
  • std::reference_wrapper causing the compiler to trigger SFINAE trait checks on RunContext while it was still an incomplete type. So replace it with raw pointer
  • Fixed some warnings caused by the use of C++20 extensions
  • Add [[nodiscard]], noexcept, explicit enum types
  • Extract RuntimeLayer to separate compilation unit

@ComixHe ComixHe marked this pull request as ready for review May 18, 2026 09:28
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the RuntimeLayer class into its own dedicated files and modernizes several components with [[nodiscard]], explicit, and noexcept qualifiers. It also improves the Cmd::exec utility by using pipe2 with O_CLOEXEC, refining environment variable handling, and ensuring proper file descriptor management. A critical issue was identified where the removal of the <list> header in run_context.h would cause a compilation error because the std::list container is still utilized within that file.

Comment thread libs/linglong/src/linglong/runtime/run_context.h
- std::reference_wrapper causing the compiler to
  trigger SFINAE trait checks on RunContext while
  it was still an incomplete type. So replace it
  with raw pointer
- Fixed some warnings caused by the use of C++20
  extensions
- Add [[nodiscard]], noexcept, explicit enum types
- Extract RuntimeLayer to separate compilation unit

Signed-off-by: Yuming He <ComixHe1895@outlook.com>
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

deepin pr auto review

这份 Git Diff 包含了大量的代码重构、性能优化、安全加固以及代码规范性的改进。整体来看,改动质量很高,显著提升了代码的健壮性。以下是我对各个维度的详细审查意见:

一、 语法与逻辑

  1. RuntimeLayerRunContext 指针空值检查(极佳)

    • 位置: libs/linglong/src/linglong/runtime/layer.cpp:49
    • 评价: 在 resolveLayer 方法中新增了对 this->runContext == nullptr 的检查。因为重构后将 std::reference_wrapper 改为了裸指针,这个空值检查是必须的,逻辑严谨,防止了空指针解引用的崩溃风险。
  2. PreparedContainer 生命周期与指针逻辑(需注意)

    • 位置: libs/linglong/src/linglong/runtime/container_builder.h:107
    • 评价: 将 std::reference_wrapper<runtime::RunContext> 改为 runtime::RunContext *runContext{ nullptr };。这简化了语法,但必须确保 PreparedContainer 实例的生命周期严格小于 RunContext 实例。从当前上下文来看,PreparedContainer 是局部临时对象,此改动是安全的。但建议在 PreparedContainer 的注释中明确说明它不拥有 runContext 的生命周期,以防后续维护者误用。
  3. GraphicsDriverInfo 初始化方式变更

    • 位置: apps/ll-driver-detect/src/nvidia_driver_detector.cpp:85
    • 评价: 从聚合初始化 { getDriverIdentify(), packageName } 改为了分别赋值。这通常意味着 GraphicsDriverInfo 可能新增了成员变量、改变了成员顺序,或者包含不可缺省的参数。逻辑上没有问题,符合 C++ 规范。

二、 代码质量

  1. 消除未使用的变量与头文件(极佳)

    • 位置: 多处,如移除 TASK_DONE 常量,移除 container_cfg_builder.hxdg/directory.h 等头文件。
    • 评价: 极大地提升了代码的整洁度,减少了编译依赖和潜在的宏污染。
  2. 现代 C++ 特性的应用(极佳)

    • = default 析构函数: RunContext::~RunContext() = default;,当类无需自定义析构逻辑时,这是最佳实践。
    • [[nodiscard]] 属性: 为 getConfig(), getBaseLayer() 等纯 getter 方法加上了 [[nodiscard]],防止调用者无意义地丢弃返回值。
    • explicit 关键字: 为 RunContext 的单参数构造函数加上了 explicit,防止隐式类型转换引发的难以察觉的 Bug。
    • std::make_unique: container.cpp 中使用 std::make_unique 替代了 new,这是更安全、更推荐的做法。
    • std::array 替代 C 数组: ostree_repo.cppconst std::array<std::string, 2> appDirs,提升了类型安全性和与 STL 算法的兼容性。
  3. 忽略未使用的返回值(佳)

    • 位置: ostree_repo.cpp:2108file.cpp:286
    • 评价: 使用 std::ignore = std::filesystem::symlink_status(...) 显式忽略返回值,向编译器和阅读者明确传达了“我们知道有返回值,但这里不需要用它”的意图,消除了编译器警告。
  4. 测试代码的规范化(佳)

    • 位置: run_context_test.cpp
    • 评价: 将 using ::testing::_; 改为在需要时使用 testing::_,避免了命名空间的污染,使测试代码更加清晰严谨。

三、 代码性能

  1. 字符串匹配性能优化(极佳)

    • 位置: libs/utils/src/linglong/utils/cmd.cpp:121-127
    • 评价: 将 env.substr(0, name.size()) == name 替换为 env.compare(0, name.size(), name) == 0。这是一个非常专业的性能优化!substr 会产生临时的字符串对象,涉及内存分配和拷贝;而 compare 的重载版本直接在原字符串上进行比较,零内存分配。在环境变量这种可能被频繁遍历调用的场景下,性能提升明显。
  2. Qt List 对象拷贝优化(佳)

    • 位置: container_builder.cpp:65
    • 评价: auto arguments = reply.arguments(); 提前将 QList 取出,而不是在 const 引用上持续操作底层容器。结合后续的 const auto &value = arguments.constFirst();,保证了数据访问的生命周期安全。

四、 代码安全

  1. 文件描述符泄露与 Fork 安全(极佳,核心改进)

    • 位置: libs/utils/src/linglong/utils/cmd.cpp
    • 评价: 这里的改动堪称全 Diff 最亮眼的安全修复:
      • **OOSTreeRepo::exportDirCmd::exec
    • 评价: 将 env.substr(0, name.size()) == name 替换为 env.compare(0, name.size(), name) == 0compare 直接在原字符串上进行比较,避免了 substr 产生的临时 std::string 对象的内存分配和拷贝开销,在频繁调用的环境变量处理场景下性能提升显著。
  2. 减少深拷贝

    • 位置: container_builder.cpp:309
    • 评价: auto arguments = reply.arguments(); 提前拷贝了一份参数列表,这是为了应对 reply.arguments().constFirst() 可能导致的悬垂引用问题(Qt的隐式共享机制在多线程或特定上下文下的风险)。虽然增加了一次浅拷贝,但保证了内存安全,权衡是合理的。

四、 代码安全

  1. 文件描述符泄露与 Fork 安全(极佳,关键改进)

    • 位置: libs/utils/src/linglong/utils/cmd.cpp
    • 评价:
      • 使用 pipe2(..., O_CLOEXEC) 替代 pipe。这确保了在执行 execvpe 时,所有未使用的管道文件描述符会自动关闭,防止了文件描述符泄露给子进程,这是非常专业的做法。
      • 在 RAII 清理器(Closer)中加入了 if (fd >= 0) 的判断,并在父进程关闭管道端后将其置为 -1。这使得文件描述符的生命周期管理极其健壮,避免了 double close(重复关闭已关闭的fd,可能被多线程并发复用导致关闭别人的fd)的严重安全漏洞。
      • 子进程退出使用 _exit(EXIT_FAILURE) 而非 exit(1)。在 fork 后的子进程中,exit 会清理 C 运行时缓冲区(如刷新 std::cout),可能导致父进程的缓冲区数据被重复刷新或损坏,_exit 是安全的做法。
  2. 环境变量覆写安全(极佳)

    • 位置: libs/utils/src/linglong/utils/cmd.cpp:120-132
    • 评价: 修复了环境变量匹配逻辑。原逻辑 env.substr(0, name.size()) == name && env[name.size()] == '=' 存在潜在的越界风险(如果 env.size() == name.size()env[name.size()] 会越界读取 \0,虽通常不崩溃但逻辑不严谨)。新逻辑 env.size() > name.size() && env.compare(0, name.size(), name) == 0 && env[name.size()] == '=' 严格保证了长度的安全检查。
  3. 临时文件清理的健壮性(佳)

    • 位置: layer.cpp:38-44
    • 评价: 在 ~RuntimeLayer() 中,为 std::filesystem::remove_all 添加了 std::error_code 捕获和日志打印。析构函数中绝对不能抛出异常,这里处理得非常规范,既清理了临时文件,又保证了程序不会因异常而崩溃。

总结

这是一次非常高质量的提交。重点解决了底层命令执行模块(Cmd)的文件描述符泄露和并发安全问题,并在全项目范围内推行了现代 C++ 的最佳实践(如 [[nodiscard]], std::make_unique, std::array, explicit 等)。代码逻辑严谨,性能优化点抓得准确。

无需进一步修改即可合入,建议合入后持续关注 PreparedContainer 中裸指针的生命周期管理规范即可。

@codecov

codecov Bot commented May 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.42017% with 59 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libs/linglong/src/linglong/runtime/layer.cpp 44.73% 4 Missing and 17 partials ⚠️
libs/linglong/src/linglong/runtime/run_context.cpp 15.38% 2 Missing and 9 partials ⚠️
...inglong/src/linglong/runtime/container_builder.cpp 0.00% 7 Missing ⚠️
...ps/ll-driver-detect/src/nvidia_driver_detector.cpp 0.00% 6 Missing ⚠️
libs/utils/src/linglong/utils/cmd.cpp 77.77% 3 Missing and 3 partials ⚠️
libs/linglong/src/linglong/runtime/run_context.h 75.00% 3 Missing ⚠️
libs/linglong/src/linglong/repo/ostree_repo.cpp 33.33% 1 Missing and 1 partial ⚠️
libs/linglong/src/linglong/runtime/container.cpp 0.00% 2 Missing ⚠️
libs/linglong/src/linglong/runtime/layer.h 90.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
libs/linglong/src/linglong/cli/cli.cpp 1.68% <ø> (ø)
libs/linglong/src/linglong/cli/cli.h 8.33% <ø> (ø)
...long/src/linglong/package_manager/package_task.cpp 50.92% <ø> (ø)
libs/utils/src/linglong/utils/file.cpp 27.95% <100.00%> (ø)
libs/linglong/src/linglong/runtime/layer.h 90.00% <90.00%> (ø)
libs/linglong/src/linglong/repo/ostree_repo.cpp 15.93% <33.33%> (ø)
libs/linglong/src/linglong/runtime/container.cpp 0.00% <0.00%> (ø)
libs/linglong/src/linglong/runtime/run_context.h 76.92% <75.00%> (+0.45%) ⬆️
...ps/ll-driver-detect/src/nvidia_driver_detector.cpp 5.83% <0.00%> (-0.21%) ⬇️
libs/utils/src/linglong/utils/cmd.cpp 59.11% <77.77%> (+2.72%) ⬆️
... and 3 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ComixHe, reddevillg

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

@reddevillg reddevillg merged commit b24be64 into OpenAtom-Linyaps:master May 19, 2026
17 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