Refactor overlay#1643
Conversation
Signed-off-by: reddevillg <reddevillg@gmail.com>
- support kernel/FUSE mode in OverlayFS - support multiple lowerdirs and optional upper/work dirs Signed-off-by: reddevillg <reddevillg@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: reddevillg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a major refactoring of the container runtime and package management system, introducing a structured RunContextConfig and a new InitRunContext DBus interface. Key changes include a new OverlayFSDriver supporting both Kernel and FUSE modes, a refactored ContainerBuilder for specialized container lifecycles, and enhanced namespace support with UID/GID mapping. Feedback identifies a potential null pointer dereference in the package manager's task handling and a race condition in the CLI process startup due to hardcoded sleeps. Additionally, the review highlights thread-safety concerns in the OverlayFS mounting logic, suggests replacing perror with structured error results in library code, and recommends ensuring consistent timezone defaults when /etc/localtime is a regular file.
| return toDBusReply(task); | ||
| } | ||
|
|
||
| auto &taskRef = task->get(); |
There was a problem hiding this comment.
The code accesses the task object without verifying if the underlying pointer is valid. If addPackageTask returns a successful result but the task pointer itself is null, this will lead to a crash when calling taskRef.taskID().
| LINGLONG_LIBEXEC_DIR "/ll-package-manager", | ||
| "--no-dbus" }); | ||
| using namespace std::chrono_literals; | ||
| std::this_thread::sleep_for(1s); |
There was a problem hiding this comment.
Using a hardcoded sleep (std::this_thread::sleep_for(1s)) to wait for a detached process to initialize is a known anti-pattern that introduces race conditions. It is better to implement a polling mechanism with a timeout that checks for the existence of the communication socket or another readiness indicator.
|
|
||
| if (dup2(stdinPipe[0], STDIN_FILENO) == -1) { | ||
| perror(("dup2 stdin failed for " + command).c_str()); | ||
| perror(fmt::format("dup2 stdin failed for {}", command).c_str()); |
There was a problem hiding this comment.
Using perror in library code is generally discouraged as it prints directly to stderr. It is better to capture the error message and return it via the utils::error::Result type, allowing the caller to handle logging appropriately.
| contextCfg.timezone = ""; | ||
| return LINGLONG_OK; | ||
| } | ||
|
|
There was a problem hiding this comment.
If /etc/localtime is a regular file instead of a symlink, the timezone is set to an empty string. This behavior differs from the case where the file is missing (where it defaults to 'UTC'). Consider if a default value like 'UTC' should be used here as well to ensure consistency.
| if (mounted_) { | ||
| LogW("OverlayFS already mounted at {}", merged_.string()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The mount() method is not thread-safe. Multiple threads calling mount() simultaneously could bypass the mounted_ check and attempt to mount the filesystem multiple times. Consider using a mutex or an atomic flag to protect the mounting state.
Changes: - Add InitRunContext D-Bus method for preparing container cache before run - Introduce OverlayFSDriver abstraction with kernel and FUSE implementations - Enter new user namespace and mount namespace before setup overlayfs - Refactor ContainerBuilder to support init, build, and run container modes - Add unit tests for OverlayFSDriver and RunContext Signed-off-by: reddevillg <reddevillg@gmail.com>
reddevillg
force-pushed
the
refactor_overlay
branch
from
52438aa to
53cb0b5
Compare
deepin pr auto review代码审查报告总体评价这次提交包含大量代码变更，主要涉及容器运行时、包管理和CLI工具的重构。代码整体质量较高，但存在一些可以改进的地方。 主要变更点
具体问题与建议1. 语法与逻辑问题问题1：在 auto pkgMan = this->getPkgMan();
if (!pkgMan) {
this->printer.printErr(pkgMan.error());
return -1;
}
auto pendingReply = (*pkgMan)->InstallFromFile(...);建议：虽然代码已经检查了返回值，但建议在 问题2：在 ContainerContext::~ContainerContext()
{
if (this->overlayFS) {
this->overlayFS->unmount();
}
if (getenv("LINGLONG_DEBUG") == nullptr && !this->bundleDir.empty()) {
std::error_code ec;
std::filesystem::remove_all(this->bundleDir, ec);
if (ec) {
LogW("failed to remove bundle directory {}: {}", this->bundleDir, ec.message());
}
}
}建议：在卸载OverlayFS后，应该检查卸载是否成功，如果失败应该记录错误。此外，建议在删除目录前先确认目录是否为空。 2. 代码质量问题问题1：在 auto ContainerBuilder::prepareContainer(runtime::RunContext &context,
ContainerMode mode,
const CommonContainerOptions &options) noexcept
-> utils::error::Result<PreparedContainer>
{
// 大量代码...
}建议：考虑将这个方法拆分成更小的函数，每个函数负责一个特定的任务，提高代码可读性和可维护性。 问题2：在 auto OverlayFSDriver::createOverlayFS(...) noexcept
-> utils::error::Result<std::unique_ptr<utils::OverlayFS>>
{
// ...
auto result = prepare(...);
if (!result) {
return LINGLONG_ERR("prepare overlayfs", result);
}
// ...
}建议：统一错误处理方式，确保所有错误都包含足够的上下文信息，便于调试。 3. 性能问题问题1：在 utils::error::Result<void> RunContext::resolve(const api::types::v1::RunContextConfig &config)
{
// 大量文件系统操作和解析...
}建议：考虑添加缓存机制，避免重复解析相同的配置。对于频繁访问的文件系统路径，可以缓存其状态。 问题2：在 auto OverlayFSDriver::detectKernelOverlaySupport() noexcept -> OverlaySupport
{
std::ifstream filesystems("/proc/filesystems");
// ...
std::ifstream version("/proc/version");
// ...
}建议：这个方法的结果在运行时不会改变，可以考虑缓存检测结果。 4. 安全问题问题1：在 auto ContainerBuilder::normalizeContainerRootfs(
const std::filesystem::path &rootfs, const api::types::v1::RunContextConfig &config) noexcept
-> utils::error::Result<void>
{
std::vector<std::filesystem::path> remove{
"/etc/localtime",
"/etc/resolv.conf",
};
for (const auto &r : remove) {
std::error_code ec;
auto target = rootfs / (r.is_absolute() ? r.relative_path() : r);
if (std::filesystem::exists(target, ec)) {
std::filesystem::remove(target, ec);
// ...
}
}
// ...
}建议：在删除文件前，应该验证文件确实是由容器管理器创建的，避免意外删除重要系统文件。可以考虑使用特定的标记或目录结构来标识这些文件。 问题2：在 if (targetUid != 0) {
auto res = prepareAmbientCapabilities();
if (!res) {
LogE("failed to prepare ambient capabilities for uid {}: {}",
targetUid,
res.error().message());
return -1;
}
if (setuid(targetUid) == -1) {
LogE("setuid failed: {}", common::error::errorString(errno));
return -1;
}
// ...
}建议：在提升权限前，应该验证调用者是否有权限执行此操作。可以考虑添加额外的审计日志，记录所有权限提升操作。 5. 其他建议
总结这次提交包含大量有价值的改进，特别是容器运行时和包管理器的重构。代码整体质量较高，但在错误处理、性能优化和安全性方面还有一些改进空间。建议在合并前重点关注上述提到的问题，特别是安全相关的部分。 |
3 participants
No description provided.