Skip to content

fix: stop app.slice before logout/shutdown to ensure clean user proce…#204

Open
yixinshark wants to merge 1 commit intolinuxdeepin:masterfrom
yixinshark:fix-stopAppSlice
Open

fix: stop app.slice before logout/shutdown to ensure clean user proce…#204
yixinshark wants to merge 1 commit intolinuxdeepin:masterfrom
yixinshark:fix-stopAppSlice

Conversation

@yixinshark
Copy link
Copy Markdown
Contributor

…ss termination

Send SIGKILL to all processes in app.slice via systemd D-Bus KillUnit before logout and shutdown.

Pms: BUG-350165,BUG-350171
Change-Id: Ibba4a31d93f107946af86b4b1e06a96a5ab4d0eb

Copy link
Copy Markdown

@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 @yixinshark, 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
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yixinshark

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

…ss termination

Send SIGKILL to all processes in app.slice via systemd D-Bus KillUnit
before logout and shutdown.

Pms: BUG-350165,BUG-350171
Change-Id: Ibba4a31d93f107946af86b4b1e06a96a5ab4d0eb
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这段代码主要是为了在系统注销和关机流程中,增加对 app.slice(通常用于管理用户应用进程的 systemd slice 单元)的强制终止功能。

以下是对该代码的审查意见,包括语法逻辑、代码质量、代码性能和代码安全方面的改进建议:

1. 语法逻辑

  • 基本正确:代码逻辑清晰,在 prepareLogoutprepareShutdown 中调用 stopAppSlice,而 stopAppSlice 通过 D-Bus 调用 systemd 的 KillUnit 方法发送信号。
  • 潜在逻辑风险 - 信号选择
    • 代码中直接使用了 SIGKILL (killUnitProcesses(APP_SLICE, SIGKILL))。
    • 问题SIGKILL 是强制杀死进程,进程无法捕获该信号进行清理(如保存临时文件、关闭套接字、释放锁等)。如果在应用正在进行数据写入时强制杀死,可能会导致数据丢失或文件损坏。
    • 建议:标准的优雅退出流程通常是先发送 SIGTERM,等待一段时间,如果进程未退出,再发送 SIGKILL。虽然注销/关机场景下速度很重要,但为了稳定性,建议评估是否必须立即使用 SIGKILL

2. 代码质量

  • 头文件包含
    • 代码中使用了 SIGKILL。在 C++ 中,标准信号宏定义位于 <csignal> (C++风格) 或 <signal.h> (C风格) 中。
    • 审查点:请确认 sessionmanager.cpp 顶部是否已经包含了 <csignal><signal.h>。如果没有,虽然某些编译器可能通过间接依赖编译通过,但这属于未定义行为,应当显式包含。
  • 错误处理
    • killUnitProcesses 函数有返回值 bool 表示成功或失败,但调用者 stopAppSlice 忽略了这个返回值。
    • 建议:虽然注销流程通常不希望因某个服务停止失败而中断,但建议至少在日志中记录失败情况,或者根据业务逻辑判断是否需要重试。
  • 常量定义
    • APP_SLICE 被定义为宏,这是 Qt/C++ 项目中的常见做法,符合现有代码风格(如 DDE_DOCK_SERVICE),没有问题。

3. 代码性能

  • 同步 D-Bus 调用
    • m_systemd1ManagerInter->KillUnit(...) 是一个同步(阻塞)调用。在注销和关机场景下,系统负载通常较高,如果 systemd 响应缓慢,会导致 UI 线程(假设这是在主线程运行的)短暂卡顿。
    • 建议:虽然注销时的短暂卡顿可能用户感知不明显,但最佳实践是使用异步调用(QDBusPendingCallWatcher)或者将此操作移至工作线程中执行,以保证 UI 的响应性(如果此时还有 UI 展示的话)。鉴于这是关机流程,同步调用在可接受范围内,但需知晓此风险。

4. 代码安全

  • 版权年份更新
    • utils.h 中的版权年份从 2023 更新到了 2026
    • 建议:通常版权年份应更新为当前年份(如 2024 或 2025)或使用 "2021 - present" 的形式。直接写 2026 可能是笔误,建议修正。
  • 权限与接口
    • 通过 D-Bus 调用 systemd 的 KillUnit 需要相应的权限。代码中使用了 m_systemd1ManagerInter,这通常意味着会与系统总线上的 systemd 通信。
    • 风险:确保调用该接口的进程具有足够的权限(通常是 root 权限或通过 polkit 授权)。如果权限不足,reply.isError() 会捕获到错误,但功能将失效。
  • 硬编码字符串
    • killUnitProcesses 中硬编码了 "all" 作为参数(即杀死 slice 下的所有进程)。这是符合预期的,但需确保 APP_SLICE 确实只包含需要被杀死的用户应用进程,避免误杀系统关键进程。

5. 改进代码示例

针对上述提到的 SIGTERM 优先和日志记录的建议,代码可以优化如下:

// sessionmanager.cpp

// 修改后的 stopAppSlice,增加优雅退出尝试
void SessionManager::stopAppSlice()
{
    const QString unit = APP_SLICE;
    
    // 1. 先尝试发送 SIGTERM (信号 15),允许进程清理
    if (!killUnitProcesses(unit, SIGTERM)) {
        qWarning() << "Failed to send SIGTERM to" << unit;
        // 如果发送 SIGTERM 失败,可能已经不存在了,或者权限问题,直接返回或继续尝试 SIGKILL 视业务需求而定
        return; 
    }

    // 2. 等待一小段时间(例如 500ms - 1000ms),让进程退出
    // 注意:这里使用了 QThread::sleep,如果是主线程请谨慎使用,最好改用 QEventLoop 或定时器
    // 但鉴于这是关机流程,简单的延时可能是可接受的
    QThread::msleep(500); 

    // 3. 检查进程是否还在,或者直接发送 SIGKILL 确保清理
    // 这里为了简单起见,直接发送 SIGKILL 确保残留进程被杀死
    // 也可以通过 systemd ListUnits 检查状态,但会增加 D-Bus 调用开销
    if (!killUnitProcesses(unit, SIGKILL)) {
        qWarning() << "Failed to send SIGKILL to" << unit;
    }
}

// killUnitProcesses 保持不变,但建议确认头文件包含
// #include <csignal>
bool SessionManager::killUnitProcesses(const QString &unit, int signal)
{
    auto reply = m_systemd1ManagerInter->KillUnit(unit, "all", signal);
    if (reply.isError()) {
        qWarning() << "failed to kill unit:" << unit << "with signal:" << signal 
                   << ", error:" << reply.error().name();
        return false;
    }
    return true;
}

总结

这段代码在功能上实现了需求,但在进程终止的优雅性(直接使用 SIGKILL)和版权年份(2026)上存在改进空间。建议增加 SIGTERM 尝试步骤以提高数据安全性,并修正版权年份。同时,请确认 <csignal> 头文件的包含。

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.

2 participants