refactor: Remove Qt dependencies from the utils#1559
Conversation
Summary of ChangesHello @reddevillg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a significant refactoring to remove Qt dependencies from the utils library, primarily by reimplementing the Cmd class. The changes are extensive and well-executed for the most part. The new Cmd implementation using POSIX APIs is a great improvement, and the addition of unit tests for it and for file operations is highly appreciated.
I've found a few issues that need attention:
- A potential compilation error due to using
QStringwithfmt::format. - A critical issue where removing
Q_ASSERTin a constructor can lead to silent failures and subsequent crashes. - An incorrect log message that seems to be a copy-paste error.
- The new
Cmdimplementation still uses the non-thread-safestrerrorfunction, which contradicts one of the goals of this PR.
Please address these points. Overall, this is a solid piece of work towards making the codebase more modular and robust.
| if (!ret) { | ||
| qCritical() << "init work dir failed"; | ||
| Q_ASSERT(false); | ||
| LogE("init work dir failed"); | ||
| } |
There was a problem hiding this comment.
The removal of Q_ASSERT(false) here means that if initWorkDir() fails, the constructor will silently continue, leaving the LayerPackager object in an invalid state (e.g., workDir is not initialized). This can lead to crashes or undefined behavior later on. A constructor that can fail should either throw an exception or you should use a factory function that returns a tl::expected<LayerPackager, ...> to handle the failure explicitly. Given the codebase's use of tl::expected, a factory function would be the idiomatic choice. The same issue exists in initWorkDir where another Q_ASSERT was removed.
reddevillg
force-pushed
the
remove_qt_1
branch
from
4491c7b to
a8f95b7
Compare
There was a problem hiding this comment.
Pull request overview
This pull request refactors the utils library to remove Qt dependencies, replacing Qt-based implementations with standard C++ alternatives. The main changes include a complete rewrite of the Cmd class for command execution, thread-safe error handling using errorString() instead of strerror(), and improved environment variable management.
Changes:
- Reimplemented
linglong::utils::Cmdusing standard C++ with fork/exec and epoll for non-blocking I/O instead of QProcess - Added
errorString()function for thread-safe errno to string conversion, replacing directstrerror()calls - Moved
EnvironmentVariableGuardand command utilities fromlinglong::utils::commandnamespace tolinglong::utils - Added
concatFile()utility function and comprehensive unit tests for file operations and command execution - Fixed bug where GIT_SUBMODULES environment variable was set for all source types instead of only git sources
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/utils/src/linglong/utils/cmd.{h,cpp} | New Cmd implementation using fork/exec/epoll replacing QProcess |
| libs/utils/src/linglong/utils/env.{h,cpp} | Moved EnvironmentVariableGuard from command namespace |
| libs/utils/src/linglong/utils/file.{h,cpp} | Updated file operations signatures and added concatFile function |
| libs/utils/src/linglong/utils/error/error.{h,cpp} | Added errorString() for thread-safe errno conversion |
| libs/utils/src/linglong/utils/command/* | Removed Qt-dependent command utilities |
| libs/linglong/src/linglong/builder/source_fetcher.cpp | Fixed to only set GIT_SUBMODULES for git sources |
| libs/linglong/tests/ll-tests/src/linglong/utils/*_test.cpp | Added comprehensive tests for new implementations |
| Multiple files | Updated imports and usage to use new linglong::utils namespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto msg = std::string("write file: ") + std::strerror(errno); | ||
| return LINGLONG_ERR(msg.c_str()); | ||
| if (out.bad()) { | ||
| return LINGLONG_ERR(fmt::format("failed to write file {}", errorString(errno))); |
There was a problem hiding this comment.
The error message is missing the filename context. It should include the filepath parameter to clearly indicate which file failed to write, similar to the error message on line 54. The format should be "failed to write file {}: {}" with filepath as the first argument.
| std::vector<char *> argv; | ||
| auto filename = commandPath.filename(); | ||
| argv.push_back(const_cast<char *>(filename.c_str())); | ||
| for (const auto &arg : args) { | ||
| argv.push_back(const_cast<char *>(arg.c_str())); | ||
| } |
There was a problem hiding this comment.
The child process is using pointers from temporary objects whose lifetime ends immediately after the statement. Specifically, filename.c_str() points to a temporary string returned by commandPath.filename(), and arg.c_str() points to elements in the args vector. After fork(), the child process uses these pointers in execvpe(), but the temporary objects may be destroyed. The argv should be built from stable strings, either by copying the filename string into a local variable or storing it in the argv vector directly.
| std::vector<char *> argv; | |
| auto filename = commandPath.filename(); | |
| argv.push_back(const_cast<char *>(filename.c_str())); | |
| for (const auto &arg : args) { | |
| argv.push_back(const_cast<char *>(arg.c_str())); | |
| } | |
| // Build stable storage for argv strings to ensure their lifetime covers execvpe | |
| std::vector<std::string> argvStrings; | |
| argvStrings.reserve(args.size() + 1); | |
| argvStrings.push_back(commandPath.filename().string()); | |
| for (const auto &arg : args) { | |
| argvStrings.push_back(arg); | |
| } | |
| std::vector<char *> argv; | |
| argv.reserve(argvStrings.size() + 1); | |
| for (auto &s : argvStrings) { | |
| argv.push_back(const_cast<char *>(s.c_str())); | |
| } |
|
|
||
| // Test writing empty content | ||
| fs::path empty_file = dest_dir / "empty.txt"; | ||
| result = linglong::utils::writeFile(empty_file.string(), "\0"); |
There was a problem hiding this comment.
The test is using a string literal "\0" which creates a string containing a null terminator followed by an implicit null terminator. This is a 2-character string, not an empty string. To test writing truly empty content, use an empty string literal "" instead of "\0". The current code tests writing a null byte, which is then read as an empty string by the iterator, making line 361 pass by accident rather than testing the intended scenario.
| result = linglong::utils::writeFile(empty_file.string(), "\0"); | |
| result = linglong::utils::writeFile(empty_file.string(), ""); |
| pid_t pid = fork(); | ||
| if (pid == -1) { | ||
| return LINGLONG_ERR(fmt::format("fork error: {}", errorString(errno))); | ||
| } | ||
|
|
||
| // child process | ||
| if (pid == 0) { | ||
| // Redirect stdout and stdin | ||
| close(stdoutPipe[0]); | ||
| close(stdinPipe[1]); | ||
| if (dup2(stdoutPipe[1], STDOUT_FILENO) == -1 || dup2(stdinPipe[0], STDIN_FILENO) == -1) { | ||
| exit(1); | ||
| } | ||
| close(stdoutPipe[1]); | ||
| close(stdinPipe[0]); | ||
|
|
||
| std::vector<char *> argv; | ||
| auto filename = commandPath.filename(); | ||
| argv.push_back(const_cast<char *>(filename.c_str())); | ||
| for (const auto &arg : args) { | ||
| argv.push_back(const_cast<char *>(arg.c_str())); | ||
| } | ||
| argv.push_back(nullptr); | ||
|
|
||
| execvpe(commandPath.c_str(), argv.data(), envp.data()); | ||
|
|
||
| exit(1); | ||
| } | ||
|
|
||
| // parent process | ||
| close(stdoutPipe[1]); | ||
| close(stdinPipe[0]); | ||
|
|
||
| auto setNonBlock = [](int fd) { | ||
| int flags = fcntl(fd, F_GETFL, 0); | ||
| if (flags != -1) { | ||
| return fcntl(fd, F_SETFL, flags | O_NONBLOCK) != -1; | ||
| } | ||
| return false; | ||
| }; | ||
| if (!setNonBlock(stdoutPipe[0]) || !setNonBlock(stdinPipe[1])) { | ||
| return LINGLONG_ERR(fmt::format("set non block error: {}", errorString(errno))); | ||
| } | ||
|
|
||
| int epfd = epoll_create1(O_CLOEXEC); | ||
| if (epfd == -1) { | ||
| return LINGLONG_ERR(fmt::format("epoll_create error: {}", errorString(errno))); | ||
| } | ||
| auto epfdCloser = utils::finally::finally([epfd]() { | ||
| close(epfd); | ||
| }); | ||
|
|
||
| struct epoll_event ev; | ||
| int activeFds = 0; | ||
|
|
||
| ev.events = EPOLLIN; | ||
| ev.data.fd = stdoutPipe[0]; | ||
| if (epoll_ctl(epfd, EPOLL_CTL_ADD, stdoutPipe[0], &ev) == -1) { | ||
| return LINGLONG_ERR(fmt::format("epoll_ctl stdout error: {}", errorString(errno))); | ||
| } | ||
| activeFds++; | ||
|
|
||
| size_t writtenBytes = 0; | ||
| if (!m_stdinContent.empty()) { | ||
| ev.events = EPOLLOUT; | ||
| ev.data.fd = stdinPipe[1]; | ||
| if (epoll_ctl(epfd, EPOLL_CTL_ADD, stdinPipe[1], &ev) == -1) { | ||
| return LINGLONG_ERR(fmt::format("epoll_ctl stdin error: {}", errorString(errno))); | ||
| } | ||
| activeFds++; | ||
| } else { | ||
| // If no input, close write end immediately to send EOF to child | ||
| close(stdinPipe[1]); | ||
| } | ||
|
|
||
| std::string output; | ||
| std::vector<char> buffer(4096); | ||
| const int MAX_EVENTS = 2; | ||
| struct epoll_event events[MAX_EVENTS]; | ||
|
|
||
| while (activeFds > 0) { | ||
| int nfds = epoll_wait(epfd, events, MAX_EVENTS, -1); | ||
| if (nfds == -1) { | ||
| if (errno == EINTR) | ||
| continue; | ||
| return LINGLONG_ERR(fmt::format("epoll_wait error: {}", errorString(errno))); | ||
| } | ||
|
|
||
| for (int i = 0; i < nfds; ++i) { | ||
| int fd = events[i].data.fd; | ||
| if (fd == stdoutPipe[0]) { | ||
| while (true) { | ||
| ssize_t n = read(fd, buffer.data(), buffer.size()); | ||
| if (n > 0) { | ||
| output.append(buffer.data(), static_cast<size_t>(n)); | ||
| continue; | ||
| } | ||
|
|
||
| if ((n == -1 && errno != EAGAIN && errno != EWOULDBLOCK) || n == 0) { | ||
| // error or EOF, stop monitoring this FD | ||
| epoll_ctl(epfd, EPOLL_CTL_DEL, fd, nullptr); | ||
| activeFds--; | ||
| } | ||
| break; | ||
| } | ||
| } else if (fd == stdinPipe[1]) { | ||
| if (writtenBytes < m_stdinContent.size()) { | ||
| ssize_t n = write(fd, | ||
| m_stdinContent.data() + writtenBytes, | ||
| m_stdinContent.size() - writtenBytes); | ||
| if (n > 0) { | ||
| writtenBytes += n; | ||
| } else if (n == -1) { | ||
| if (errno != EAGAIN && errno != EWOULDBLOCK) { | ||
| epoll_ctl(epfd, EPOLL_CTL_DEL, fd, nullptr); | ||
| activeFds--; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (writtenBytes >= m_stdinContent.size()) { | ||
| epoll_ctl(epfd, EPOLL_CTL_DEL, fd, nullptr); | ||
| close(fd); | ||
| activeFds--; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| int status; | ||
| while (waitpid(pid, &status, 0) == -1) { | ||
| if (errno == EINTR) | ||
| continue; | ||
| return LINGLONG_ERR(fmt::format("waitpid error: {}", errorString(errno))); | ||
| } | ||
|
|
||
| if (WIFEXITED(status)) { | ||
| int exitCode = WEXITSTATUS(status); | ||
| if (exitCode == 0) { | ||
| return output; | ||
| } else { | ||
| return LINGLONG_ERR("command execute failed", exitCode); | ||
| } | ||
| } else if (WIFSIGNALED(status)) { | ||
| return LINGLONG_ERR("command killed by signal", WTERMSIG(status)); | ||
| } | ||
|
|
||
| return LINGLONG_ERR("command exited abnormally"); |
There was a problem hiding this comment.
There's a potential resource leak if an error occurs after fork() but before waitpid(). If an error is returned (e.g., from setNonBlock or epoll operations), the child process will continue running as a zombie until the parent process exits. The code should ensure the child process is reaped in all error paths. Consider adding a finally guard after the fork to waitpid() on the child if an error occurs, or use RAII to manage the child process lifecycle.
| ofs << ifs.rdbuf(); | ||
| if (ofs.bad()) { | ||
| return LINGLONG_ERR( | ||
| fmt::format("failed to write target {} {}", target, errorString(errno))); |
There was a problem hiding this comment.
The error message is missing a colon or separator between the filepath and the error string. The format should be "failed to write target {}: {}" to match the pattern used in other error messages in the codebase.
| buffer << in.rdbuf(); | ||
| if (in.fail()) { | ||
| auto msg = std::string("read file: ") + std::strerror(errno); | ||
| if (buffer.bad()) { | ||
| auto msg = std::string("read file: ") + errorString(errno); | ||
| return LINGLONG_ERR(msg.c_str()); | ||
| } |
There was a problem hiding this comment.
Checking the stream's bad() state after reading from rdbuf() is incorrect. The bad() flag is set only for serious errors like hardware failure. After reading with operator<<(rdbuf()), you should check the input stream's fail() state instead. If the input stream fails during reading, the output stringstream won't know about it. The check should be if (in.fail()) or check both streams appropriately.
| if (std::filesystem::exists(target, ec) && std::filesystem::equivalent(source, target, ec)) { | ||
| return LINGLONG_ERR("source and target are the same file", ec); |
There was a problem hiding this comment.
The error checking logic is incomplete. After line 73, if std::filesystem::equivalent fails (ec is set), the error is silently ignored. The function should check if ec is set after calling equivalent() and handle it appropriately. Additionally, if target doesn't exist, equivalent() will fail and set ec, but this is a valid case. Consider restructuring: first check if target exists, and only if it does, check if it's equivalent to source.
| ofs << ifs.rdbuf(); | ||
| if (ofs.bad()) { | ||
| return LINGLONG_ERR( | ||
| fmt::format("failed to write target {} {}", target, errorString(errno))); | ||
| } |
There was a problem hiding this comment.
After a stream write operation like ofs << ifs.rdbuf(), checking errno may not provide meaningful error information because stream operations don't necessarily set errno. The stream's own error state should be checked instead. Additionally, bad() only detects serious stream errors. Consider checking fail() which catches both bad() and logical errors. If errno is needed, consider using strerror immediately after a failed system call rather than after stream operations.
reddevillg
force-pushed
the
remove_qt_1
branch
from
a8f95b7 to
202e174
Compare
Changes: - Reimplement `linglong::utils::Cmd`. - Add unit tests for the new `Cmd` implementation and file operations. - Use thred-safe errorString instead of strerror. - submodules only affects git kind source. - Remove unused code. Signed-off-by: reddevillg <reddevillg@gmail.com>
reddevillg
force-pushed
the
remove_qt_1
branch
from
202e174 to
0417e59
Compare
deepin pr auto reviewGit Diff 代码审查报告总体评价这次代码变更主要涉及命令执行工具的重构，将基于Qt的 代码质量与逻辑分析1. 命令执行工具重构优点：
改进建议：
// 在Cmd::exec中，建议添加环境变量清理
// 防止命令注入攻击
std::vector<std::string> envStrings;
std::vector<char *> envp;
// 添加以下代码清理潜在危险的环境变量
const char* dangerous_envs[] = {"LD_PRELOAD", "LD_LIBRARY_PATH", "IFS"};
for (char **env = environ; *env != nullptr; ++env) {
bool is_dangerous = false;
for (const char* dangerous : dangerous_envs) {
if (strncmp(*env, dangerous, strlen(dangerous)) == 0 &&
(*env)[strlen(dangerous)] == '=') {
is_dangerous = true;
break;
}
}
if (!is_dangerous) {
envStrings.emplace_back(*env);
}
}
// 在Cmd::exec中，建议添加更详细的错误信息
if (WIFSIGNALED(status)) {
int signal = WTERMSIG(status);
return LINGLONG_ERR(fmt::format(
"command killed by signal {} ({}): {}",
signal,
strsignal(signal),
output
));
}
// 在fork前添加资源检查
if (m_stdinContent.size() > 1024 * 1024 * 10) { // 限制10MB
return LINGLONG_ERR("stdin content too large, potential DoS risk");
}2. 错误处理统一优点：
改进建议：
// 建议添加错误信息翻译支持
std::string errorString(int err) {
// 可以添加翻译逻辑
return std::system_category().message(err);
}3. 文件操作改进优点：
改进建议：
// 在concatFile中添加符号链接检查
if (std::filesystem::is_symlink(source) || std::filesystem::is_symlink(target)) {
return LINGLONG_ERR("symbolic links are not allowed");
}
// 在concatFile中添加进度回调
linglong::utils::error::Result<void> concatFile(
const std::filesystem::path &source,
const std::filesystem::path &target,
std::function<void(size_t, size_t)> progressCallback = nullptr
) {
// 实现中添加进度报告
size_t totalSize = std::filesystem::file_size(source);
size_t copied = 0;
// ... 复制过程中更新进度
if (progressCallback) {
progressCallback(copied, totalSize);
}
}4. 测试代码改进优点：
改进建议：
// 添加更多边界条件测试
TEST(command, LargeStdin) {
// 测试大输入数据
std::string large_data(1024 * 1024, 'A'); // 1MB
auto ret = linglong::utils::Cmd("cat").toStdin(large_data).exec();
EXPECT_TRUE(ret);
EXPECT_EQ(*ret, large_data);
}
TEST(command, SpecialCharacters) {
// 测试特殊字符处理
std::string special_chars = "$`\\\"'()[]{}*?|&;<>";
auto ret = linglong::utils::Cmd("echo").exec({special_chars});
EXPECT_TRUE(ret);
EXPECT_EQ(*ret, special_chars + "\n");
}
TEST(command, ConcurrentExecution) {
// 测试并发执行
std::vector<std::future<linglong::utils::error::Result<std::string>>> results;
for (int i = 0; i < 10; ++i) {
results.push_back(std::async(std::launch::async, []() {
return linglong::utils::Cmd("echo").exec({std::to_string(i)});
}));
}
for (auto& result : results) {
auto ret = result.get();
EXPECT_TRUE(ret);
}
}性能优化建议
// 添加命令路径缓存
class Cmd {
private:
static std::unordered_map<std::string, std::filesystem::path> commandCache;
std::filesystem::path getCommandPath() {
auto it = commandCache.find(m_command);
if (it != commandCache.end()) {
return it->second;
}
// ... 原有查找逻辑
auto path = /* 查找结果 */;
commandCache[m_command] = path;
return path;
}
};
// 添加批量执行接口
static utils::error::Result<std::vector<std::string>> execBatch(
const std::vector<std::pair<std::string, std::vector<std::string>>> &commands
) {
std::vector<std::string> results;
for (const auto &cmd : commands) {
auto ret = Cmd(cmd.first).exec(cmd.second);
if (!ret) {
return LINGLONG_ERR(ret.error());
}
results.push_back(*ret);
}
return results;
}安全性建议
// 添加命令白名单机制
class Cmd {
private:
static std::unordered_set<std::string> allowedCommands;
public:
static void setAllowedCommands(const std::unordered_set<std::string> &commands) {
allowedCommands = commands;
}
utils::error::Result<std::string> exec(const std::vector<std::string> &args) noexcept {
if (!allowedCommands.empty() && allowedCommands.find(m_command) == allowedCommands.end()) {
return LINGLONG_ERR(fmt::format("command {} is not allowed", m_command));
}
// ... 原有逻辑
}
};
// 在Cmd::exec中添加资源限制
struct rlimit limit;
limit.rlim_cur = 30; // 30秒CPU时间
limit.rlim_max = 30;
if (setrlimit(RLIMIT_CPU, &limit) == -1) {
return LINGLONG_ERR(fmt::format("setrlimit failed: {}", errorString(errno)));
}总结这次代码变更整体质量较高，主要改进了命令执行工具的实现方式，使其更加轻量和高效。建议重点关注以下几个方面：
这些改进将使代码更加健壮、安全和高效。 |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Changes:
linglong::utils::Cmd.Cmdimplementation and file operations.