refactor: Replace popen with QProcess for process count and kill oper…#422
Closed
LiHua000 wants to merge 1 commit intolinuxdeepin:release/eaglefrom
Closed
refactor: Replace popen with QProcess for process count and kill oper…#422LiHua000 wants to merge 1 commit intolinuxdeepin:release/eaglefrom
LiHua000 wants to merge 1 commit intolinuxdeepin:release/eaglefrom
Conversation
…ations - Updated `getProcessCountByName` to use `QProcess` for executing the `ps` command, improving reliability and handling of process output. - Refactored `killProcessByName` to utilize `QProcess` for killing processes, enhancing code consistency and error handling. - Improved null and empty string checks for process names in both functions. This change enhances the maintainability and performance of process management in the application.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiHua000 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 |
Reviewer's GuideRefactors process management helpers to use Qt's QProcess instead of C stdlib process APIs, adding logging, stricter argument validation, and synchronous execution with timeouts for ps and killall invocations. Sequence diagram for getProcessCountByName using QProcesssequenceDiagram
participant Caller
participant Utils
participant QProcess
participant OS_ps
Caller->>Utils: getProcessCountByName(pstrName)
Utils->>Utils: check pstrName null_or_empty
alt pstrName_invalid
Utils-->>Caller: return -1
else pstrName_valid
Utils->>QProcess: start(ps, ["-ef"])
QProcess->>OS_ps: execute ps -ef
OS_ps-->>QProcess: process_list_output
Utils->>QProcess: waitForFinished(5000)
alt finished_in_time
QProcess-->>Utils: stdout
Utils->>Utils: parse_output_and_count_matches
Utils-->>Caller: return count
else timeout
Utils-->>Caller: return -1
end
end
Sequence diagram for killProcessByName using QProcesssequenceDiagram
participant Caller
participant Utils
participant QProcess
participant OS_killall
Caller->>Utils: killProcessByName(pstrName)
Utils->>Utils: check pstrName null_or_empty
alt pstrName_invalid
Utils-->>Caller: return
else pstrName_valid
Utils->>QProcess: start(killall, [pstrName])
QProcess->>OS_killall: execute killall pstrName
OS_killall-->>QProcess: result
Utils->>QProcess: waitForFinished(5000)
QProcess-->>Utils: exit_status
Utils-->>Caller: return
end
Class diagram for Utils process management refactorclassDiagram
class Utils {
+int getProcessCountByName(const char* pstrName)
+void killProcessByName(const char* pstrName)
}
class QProcess {
+void start(QString program, QStringList arguments)
+bool waitForFinished(int msecs)
+QByteArray readAllStandardOutput()
}
Utils ..> QProcess: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这段代码将原有的 C 风格系统调用（ 1. 语法与逻辑
2. 代码质量
3. 代码性能
4. 代码安全
5. 改进后的代码示例针对上述问题，提供一个优化后的版本： #include <QProcess>
#include <QRegularExpression>
#include <QDebug>
int Utils::getProcessCountByName(const char *pstrName)
{
// 使用分类日志或条件编译，避免生产环境性能损耗
// qDebug() << "Enter getProcessCountByName, pstrName:" << pstrName;
if (pstrName == NULL || strlen(pstrName) == 0) {
// qDebug() << "pstrName is null";
return -1;
}
QProcess process;
// 使用 ps -e 仅显示进程ID和命令，减少输出量，或者保持 -ef 以获取更多上下文
process.start("ps", QStringList() << "-e" << "-o" << "comm="); // 仅获取进程名
if (!process.waitForFinished(5000)) {
qDebug() << "ps command timeout or failed";
return -1;
}
if (process.exitCode() != 0) {
qDebug() << "ps command failed with code:" << process.exitCode();
return -1;
}
QByteArray output = process.readAllStandardOutput();
QList<QByteArray> lines = output.split('\n');
QString processName = QString::fromLocal8Bit(pstrName);
int count = 0;
// 使用精确匹配，或者根据需求调整正则
// 注意：ps -o comm= 通常只输出进程名（不含路径），长度限制为15个字符
// 如果需要完整命令行，需调整 ps 参数和匹配逻辑
for (const QByteArray &line : lines) {
QString lineStr = QString::fromLocal8Bit(line).trimmed();
if (!lineStr.isEmpty() && lineStr == processName) {
count++;
}
}
// qDebug() << "Exit getProcessCountByName, count:" << count;
return count;
}
void Utils::killProcessByName(const char *pstrName)
{
// qDebug() << "Enter killProcessByName, pstrName:" << pstrName;
if (pstrName == NULL || strlen(pstrName) == 0) {
// qDebug() << "pstrName is null or empty";
return;
}
QProcess process;
process.start("killall", QStringList() << QString::fromLocal8Bit(pstrName));
if (!process.waitForFinished(5000)) {
qDebug() << "killall command timeout";
} else {
if (process.exitCode() != 0) {
QByteArray err = process.readAllStandardError();
qDebug() << "killall failed:" << err;
}
}
// qDebug() << "Exit killProcessByName";
}总结这次修改主要解决了安全性问题（命令注入），但在功能准确性（进程名匹配）和性能（大数据量文本处理）上还有提升空间。建议根据实际运行环境和需求，选择更精确的进程匹配方式，并考虑日志对性能的影响。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
getProcessCountByNamelogic changes semantics from the originalps | grep | grep -v grep | wc -l(which excluded the grep process and matched word boundaries) to a simpleline.contains(processName), which may overcount (e.g., substrings in args or the ps header); consider using stricter matching (e.g., regex on the process column) and explicitly skipping the header line. - Both
getProcessCountByNameandkillProcessByNameblock synchronously onwaitForFinished(5000)without checkingQProcess::exitStatus()/exitCode(), so failures and timeouts are silently treated as success; consider checking and logging the process result or returning an error code to callers. QString::fromUtf8(pstrName)assumes the incomingchar*is UTF-8; if this comes from the local 8-bit environment (e.g., argv or system APIs),QString::fromLocal8Bitmay be more appropriate to avoid mis-decoding process names on non-UTF-8 systems.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `getProcessCountByName` logic changes semantics from the original `ps | grep | grep -v grep | wc -l` (which excluded the grep process and matched word boundaries) to a simple `line.contains(processName)`, which may overcount (e.g., substrings in args or the ps header); consider using stricter matching (e.g., regex on the process column) and explicitly skipping the header line.
- Both `getProcessCountByName` and `killProcessByName` block synchronously on `waitForFinished(5000)` without checking `QProcess::exitStatus()`/`exitCode()`, so failures and timeouts are silently treated as success; consider checking and logging the process result or returning an error code to callers.
- `QString::fromUtf8(pstrName)` assumes the incoming `char*` is UTF-8; if this comes from the local 8-bit environment (e.g., argv or system APIs), `QString::fromLocal8Bit` may be more appropriate to avoid mis-decoding process names on non-UTF-8 systems.
## Individual Comments
### Comment 1
<location> `src/common/utils.cpp:758-765` </location>
<code_context>
- memset(command, 0, sizeof(command));
- sprintf(command, "ps -ef | grep %s | grep -v grep | wc -l", pstrName);
+ QProcess process;
+ process.start("ps", QStringList() << "-ef");
+ if (!process.waitForFinished(5000)) {
+ qDebug() << "ps command timeout";
+ return -1;
+ }
+
+ QString output = process.readAllStandardOutput();
+ QString processName = QString::fromUtf8(pstrName);
+ int count = 0;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle ps execution errors and non-zero exit statuses explicitly.
Currently only the timeout is handled; all other outcomes are treated as success. Please also check `process.error()`, `process.exitStatus()`, and `process.exitCode()`, and return `-1` (or another explicit error code) when `ps` fails instead of proceeding to parse a potentially empty or partial `output` buffer.
```suggestion
QProcess process;
process.start("ps", QStringList() << "-ef");
if (!process.waitForFinished(5000)) {
qDebug() << "ps command timeout";
return -1;
}
if (process.error() != QProcess::UnknownError) {
qDebug() << "ps command failed, error:" << process.errorString();
return -1;
}
if (process.exitStatus() != QProcess::NormalExit) {
qDebug() << "ps command did not exit normally, exitStatus:" << process.exitStatus();
return -1;
}
if (process.exitCode() != 0) {
qDebug() << "ps command exited with non-zero code:" << process.exitCode();
return -1;
}
QString output = process.readAllStandardOutput();
```
</issue_to_address>
### Comment 2
<location> `src/common/utils.cpp:759-760` </location>
<code_context>
- memset(command, 0, sizeof(command));
- sprintf(command, "ps -ef | grep %s | grep -v grep | wc -l", pstrName);
+ QProcess process;
+ process.start("ps", QStringList() << "-ef");
+ if (!process.waitForFinished(5000)) {
+ qDebug() << "ps command timeout";
+ return -1;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider cleaning up or killing the ps process on timeout instead of just returning.
Since we return `-1` on timeout without explicitly stopping `ps`, it may keep running unnecessarily. Even though `QProcess` will eventually be destroyed, it’s clearer and safer to explicitly `terminate()` (and, if needed after a short wait, `kill()`) the process when `waitForFinished` times out.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+758
to
+765
| QProcess process; | ||
| process.start("ps", QStringList() << "-ef"); | ||
| if (!process.waitForFinished(5000)) { | ||
| qDebug() << "ps command timeout"; | ||
| return -1; | ||
| } | ||
|
|
||
| QString output = process.readAllStandardOutput(); |
There was a problem hiding this comment.
suggestion (bug_risk): Handle ps execution errors and non-zero exit statuses explicitly.
Currently only the timeout is handled; all other outcomes are treated as success. Please also check process.error(), process.exitStatus(), and process.exitCode(), and return -1 (or another explicit error code) when ps fails instead of proceeding to parse a potentially empty or partial output buffer.
Suggested change
| QProcess process; | |
| process.start("ps", QStringList() << "-ef"); | |
| if (!process.waitForFinished(5000)) { | |
| qDebug() << "ps command timeout"; | |
| return -1; | |
| } | |
| QString output = process.readAllStandardOutput(); | |
| QProcess process; | |
| process.start("ps", QStringList() << "-ef"); | |
| if (!process.waitForFinished(5000)) { | |
| qDebug() << "ps command timeout"; | |
| return -1; | |
| } | |
| if (process.error() != QProcess::UnknownError) { | |
| qDebug() << "ps command failed, error:" << process.errorString(); | |
| return -1; | |
| } | |
| if (process.exitStatus() != QProcess::NormalExit) { | |
| qDebug() << "ps command did not exit normally, exitStatus:" << process.exitStatus(); | |
| return -1; | |
| } | |
| if (process.exitCode() != 0) { | |
| qDebug() << "ps command exited with non-zero code:" << process.exitCode(); | |
| return -1; | |
| } | |
| QString output = process.readAllStandardOutput(); |
Comment on lines
+759
to
+760
| process.start("ps", QStringList() << "-ef"); | ||
| if (!process.waitForFinished(5000)) { |
There was a problem hiding this comment.
suggestion (bug_risk): Consider cleaning up or killing the ps process on timeout instead of just returning.
Since we return -1 on timeout without explicitly stopping ps, it may keep running unnecessarily. Even though QProcess will eventually be destroyed, it’s clearer and safer to explicitly terminate() (and, if needed after a short wait, kill()) the process when waitForFinished times out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ations
getProcessCountByNameto useQProcessfor executing thepscommand, improving reliability and handling of process output.killProcessByNameto utilizeQProcessfor killing processes, enhancing code consistency and error handling.This change enhances the maintainability and performance of process management in the application.
Summary by Sourcery
Refactor process management utilities to use Qt's QProcess instead of C standard library calls for counting and killing processes.
Bug Fixes:
Enhancements: