Skip to content

Comments

refactor: Replace popen with QProcess for process count and kill oper…#419

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
dengzhongyuan365-dev:master
Feb 2, 2026
Merged

refactor: Replace popen with QProcess for process count and kill oper…#419
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
dengzhongyuan365-dev:master

Conversation

@dengzhongyuan365-dev
Copy link
Contributor

…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.

…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.
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Sorry @dengzhongyuan365-dev, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的修改将原本使用 C 标准库 (popen, system) 的实现改为了使用 Qt 框架提供的 QProcess 类。这是一个好的方向,因为它提高了跨平台兼容性,并增强了代码与 Qt 生态系统的集成。

以下是对修改后代码的详细审查,包括语法逻辑、代码质量、代码性能和代码安全方面的改进建议。

1. 代码逻辑与功能

getProcessCountByName 函数:

  • 逻辑缺陷(严重):

    • 问题: 原代码使用了 grep -v grep 来过滤掉 grep 命令本身的进程。新代码移除了这个逻辑,直接在 ps 的输出中查找包含 pstrName 的行。
    • 后果: 如果用户搜索名为 "test" 的进程,而系统中有另一个名为 "my_tester" 的进程,新代码会错误地将其计入总数。此外,QProcess 自身(或其子进程)也有可能出现在进程列表中,导致计数不准。
    • 改进建议: 需要恢复过滤逻辑。不要简单使用 contains,应该解析 ps -ef 的每一行,提取真正的进程名(通常是最后一列,但有时路径较长,需要更复杂的解析),或者至少确保匹配的是完整的进程名称/路径。
  • 边界检查:

    • 改进点: 增加了对 pstrName 为空或 NULL 的检查,这很好。
    • 细节: QString::fromUtf8(pstrName) 的使用是安全的,但如果 pstrName 包含无效的 UTF-8 序列,可能会发生转换问题(虽然对于进程名通常是 ASCII 字符,风险较小)。

killProcessByName 函数:

  • 逻辑改进:
    • 改进点: 使用 killall 替代 system 调用,避免了 shell 注入的风险(详见安全部分)。
    • 逻辑修正: 原代码的 if 判断逻辑有点绕,新代码将空值检查提前并直接 return,逻辑更清晰。

2. 代码质量

  • 可读性:

    • 新代码利用了 Qt 的容器和字符串操作,比 C 风格的字符串拼接和指针操作更易读。
    • waitForFinished(5000) 增加了超时机制,防止进程卡死,这是一个很好的健壮性改进。
  • 错误处理:

    • getProcessCountByName 中,process.start 失败或超时返回了 -1,处理得当。
    • killProcessByName 中,没有检查 killall 是否执行成功或是否找到了进程。waitForFinished 的返回值或 exitCode 应该被检查,并打印日志,以便调试。

3. 代码性能

  • 内存分配:
    • 问题: output.split('\n') 会一次性将整个 ps 输出加载到内存并创建一个 QStringList。如果系统进程非常多,这会消耗较多内存和 CPU 时间。
    • 改进建议: 考虑使用 process.readLine() 逐行读取输出,这样内存占用更小,且可以在读取过程中直接处理,无需等待 ps 命令完全结束。

4. 代码安全

  • 命令注入风险(已修复):

    • 原代码: sprintf(command, "...%s...", pstrName) 存在严重的命令注入风险。如果 pstrName"myapp; rm -rf /",系统将执行灾难性命令。
    • 新代码: 使用 process.start("killall", QStringList() << ...),参数会被正确转义,完全消除了命令注入的风险。这是非常重要的安全改进。
  • getProcessCountByName 的匹配风险:

    • 由于使用了 contains,如果进程名是用户输入的,且没有进行严格的校验,可能会匹配到非预期的文件(如前所述的逻辑缺陷)。

综合改进代码示例

结合以上分析,建议对代码进行如下进一步优化:

#include <QProcess>
#include <QStringList>
#include <QDebug>

int Utils::getProcessCountByName(const char *pstrName)
{
    qDebug() << "Enter getProcessCountByName, pstrName:" << pstrName;

    if (pstrName == NULL || strlen(pstrName) == 0) {
        qDebug() << "pstrName is null or empty";
        return -1;
    }

    QProcess process;
    // 使用 ps -e 仅显示进程名,或者 ps -ef 显示完整信息
    // 这里为了准确匹配,建议解析 ps 的输出
    process.start("ps", QStringList() << "-e"); // 或者 "-eo comm=" 仅获取命令名
    if (!process.waitForFinished(5000)) {
        qDebug() << "ps command timeout or failed";
        return -1;
    }

    QByteArray output = process.readAllStandardOutput();
    QString processName = QString::fromUtf8(pstrName);
    int count = 0;

    // 逐行处理,避免 split 产生的大内存分配
    QList<QByteArray> lines = output.split('\n');
    for (const QByteArray &line : lines) {
        QString lineStr = QString::fromUtf8(line).trimmed();
        if (lineStr.isEmpty()) continue;

        // 简单的包含匹配仍然存在子串匹配问题
        // 理想情况下应该解析出完整的进程名进行全等比较
        // 这里假设 ps -e 的输出格式,最后一列是进程名(实际情况可能更复杂)
        // 如果使用 ps -eo comm=,则整行就是进程名
        if (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;
    // 注意:killall 并非所有系统都默认可用(如某些最小化 Linux 发行版或默认的 macOS 行为不同)
    // 但对于大多数桌面应用场景是可以接受的
    process.start("killall", QStringList() << QString::fromUtf8(pstrName));
    
    // 检查执行结果
    if (!process.waitForFinished(5000)) {
        qDebug() << "killall command timeout";
    } else {
        int exitCode = process.exitCode();
        if (exitCode != 0) {
            // exitCode 1 通常表示没有找到进程
            qDebug() << "killall finished with exit code:" << exitCode << process.readAllStandardError();
        } else {
            qDebug() << "Process killed successfully";
        }
    }

    qDebug() << "Exit killProcessByName";
}

总结建议

  1. 修复匹配逻辑:在 getProcessCountByName 中,不要使用 contains,应改为解析 ps 输出中的特定列(如命令名)并进行精确匹配(==),或者使用正则表达式匹配单词边界(\bname\b),以避免部分匹配导致的误报。
  2. 优化内存使用:虽然 split 在现代机器上通常不是瓶颈,但逐行读取(readLine)是处理流式数据的更好实践。
  3. 增强错误反馈:在 killProcessByName 中增加对 exitCode 的检查,区分"超时"、"未找到进程"和"成功杀死"的情况,有助于运维和调试。
  4. 跨平台考虑killall 在 Linux 上很常见,但在 Windows 上不存在。如果项目需要跨平台支持,这里需要添加预编译指令(#ifdef Q_OS_WIN)并使用 taskkill /IM ... 替代。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev, lzwind

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

@dengzhongyuan365-dev
Copy link
Contributor Author

/forcemerge

@deepin-bot deepin-bot bot merged commit c3d979e into linuxdeepin:master Feb 2, 2026
21 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