feat: add OOM score adjustment tool for critical DDE services #180
Conversation
Reviewer's GuideIntroduces a new dde-session-launcher helper binary that starts dde-session under systemd with controlled CAP_SYS_RESOURCE behavior and oom_score_adj adjustments, wires it into the build and packaging system, and prepares service/packaging changes so systemd runs dde-session via this capability-managed launcher instead of directly. Sequence diagram for dde-session-launcher process and oom_score_adj managementsequenceDiagram
actor Systemd
participant Launcher as dde_session_launcher
participant DdeSession as dde_session
participant Adjuster as adjuster_child
participant ProcFS as proc_oom_score_adj
Systemd->>Launcher: start dde_session_launcher
activate Launcher
Launcher->>Launcher: fork() for dde_session
alt fork_failed
Launcher->>Systemd: exit 1
else child_process
activate DdeSession
DdeSession->>DdeSession: execl(/usr/bin/dde-session --systemd-service)
note over DdeSession: runs as session process
else parent_process
Launcher->>Launcher: sleep(2)
Launcher->>DdeSession: kill(pid, 0) (check alive)
alt dde_session_not_running
Launcher->>Systemd: exit 1
else alive
Launcher->>ProcFS: read /proc/dde_session_pid/oom_score_adj
Launcher->>Launcher: fork() for adjuster
alt adjuster_fork_failed
Launcher->>DdeSession: SIGTERM
Launcher->>Systemd: exit 1
else adjuster_child
activate Adjuster
Adjuster->>Adjuster: setpgid(0, 0)
Adjuster->>ProcFS: write /proc/dde_session_pid/oom_score_adj = -999
alt write_failed
Adjuster->>Launcher: exit 1 (via waitpid)
else write_ok
Adjuster->>Launcher: exit 0 (via waitpid)
end
deactivate Adjuster
end
activate Launcher
Launcher->>Launcher: waitpid(adjuster_pid)
alt adjuster_success
Launcher->>ProcFS: read /proc/dde_session_pid/oom_score_adj
Launcher->>Launcher: verify value == -500
else adjuster_failed
Launcher->>Launcher: log failure
end
Launcher->>Launcher: waitpid(dde_session_pid)
Launcher->>Systemd: exit with dde_session status
deactivate Launcher
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The launcher writes
targetScore = -999tooom_score_adjbut later validates against-500, so the built-in self-test will always report failure; align the written value and expected value. - The launcher currently hardcodes
/usr/bin/dde-sessionand a fixedsleep(2)to wait for initialization, which is brittle; consider making the executable path configurable and replacing the fixed sleep with a readiness check (e.g., polling/procor checking thatoom_score_adjis accessible). - You add
libcapas a pkg-config dependency todde-sessionbut the new launcher, which is implementing capability-related behavior, doesn’t use or link against it; double-check whether the dependency should instead be associated with the launcher or removed if unused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The launcher writes `targetScore = -999` to `oom_score_adj` but later validates against `-500`, so the built-in self-test will always report failure; align the written value and expected value.
- The launcher currently hardcodes `/usr/bin/dde-session` and a fixed `sleep(2)` to wait for initialization, which is brittle; consider making the executable path configurable and replacing the fixed sleep with a readiness check (e.g., polling `/proc` or checking that `oom_score_adj` is accessible).
- You add `libcap` as a pkg-config dependency to `dde-session` but the new launcher, which is implementing capability-related behavior, doesn’t use or link against it; double-check whether the dependency should instead be associated with the launcher or removed if unused.
## Individual Comments
### Comment 1
<location> `src/dde-session-launcher/dde-session-launcher.cpp:164-167` </location>
<code_context>
+ qInfo() << "";
+ if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
+ // 验证结果
+ int newValue = readOomScore(ddeSessionPid);
+ if (newValue != -9999) {
+ qInfo() << "dde-session's new oom_score_adj:" << newValue;
+ if (newValue == -500) {
+ qInfo() << "✓ Test PASSED";
+ } else {
</code_context>
<issue_to_address>
**issue (bug_risk):** The verification expects -500 but the adjuster writes -999, so the check will always fail on success.
The adjuster writes `targetScore = -999` to `oom_score_adj`, but the verification checks `newValue == -500`. On success `newValue` will be `-999`, so this code will always treat a correct write as a failure. Align the written value and the expected value in the check so this path reports the outcome correctly.
</issue_to_address>
### Comment 2
<location> `src/dde-session-launcher/dde-session-launcher.cpp:159` </location>
<code_context>
+
+ // 父进程：等待调整器完成
+ int status;
+ waitpid(adjusterPid, &status, 0);
+
+ qInfo() << "";
</code_context>
<issue_to_address>
**suggestion:** waitpid calls do not handle EINTR, which can cause spurious failures or zombies.
Both `waitpid(adjusterPid, &status, 0);` and the later `waitpid(ddeSessionPid, &status, 0);` should loop on `EINTR`. If a signal arrives, `waitpid` can return `-1` with `errno == EINTR`, and the code will treat the child as reaped, risking zombies and skipped status handling. Please wrap these calls in a loop that retries when `errno == EINTR`.
</issue_to_address>
### Comment 3
<location> `src/dde-session-launcher/README.md:51` </location>
<code_context>
+### 方式1：直接运行（不会继承 file capabilities）
+
+```bash
+./build/src/dde-session-launcher/dde-session-launcher
+```
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The example run path is inconsistent with the earlier build steps and likely incorrect.
Since the build instructions `cd build`, the binary path from there should be `./src/dde-session-launcher/dde-session-launcher`. Using `./build/src/...` would resolve to `build/build/...` and likely fail. Please update this (and similar examples) to match the actual working directory after the build step.
</issue_to_address>
### Comment 4
<location> `src/dde-session-launcher/dde-session-launcher.cpp:77` </location>
<code_context>
+ }
+}
+
+int main(int argc, char *argv[])
+{
+ QCoreApplication app(argc, argv);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the launcher by extracting helpers, centralizing oom_score_adj handling, and simplifying logging to keep behavior while making the control flow easier to follow.
You can keep all current behavior but significantly cut complexity with a few focused refactors:
---
### 1. Remove magic sentinel and unify `/proc` path construction
Right now `-9999` is used as an error sentinel and `/proc/%d/oom_score_adj` is copy-pasted in three places. You can centralize this without changing behavior:
```c++
namespace {
constexpr int kOomReadError = std::numeric_limits<int>::min();
inline QString oomScoreAdjPath(pid_t pid)
{
return QString("/proc/%1/oom_score_adj").arg(pid);
}
} // namespace
static int readOomScore(pid_t pid)
{
const QByteArray path = oomScoreAdjPath(pid).toLocal8Bit();
char buf[32];
int fd = open(path.constData(), O_RDONLY);
if (fd < 0) {
return kOomReadError;
}
ssize_t n = read(fd, buf, sizeof(buf) - 1);
close(fd);
if (n <= 0) {
return kOomReadError;
}
buf[n] = '\0';
return atoi(buf);
}
static bool writeOomScore(pid_t pid, int oomScore)
{
const QByteArray path = oomScoreAdjPath(pid).toLocal8Bit();
char buf[32];
int fd = open(path.constData(), O_WRONLY);
// ...
}
```
Then replace `"/proc/%d/oom_score_adj"` usages and `-9999` checks with `oomScoreAdjPath()` and `kOomReadError`.
---
### 2. Extract small helpers from `main` to reduce linear complexity
`main` mixes process management, oom operations, and test reporting. Pulling out small functions keeps the control flow obvious while preserving the current forking model:
```c++
static pid_t startDdeSession()
{
pid_t pid = fork();
if (pid < 0) {
qCritical() << "Failed to fork:" << strerror(errno);
return -1;
}
if (pid == 0) {
execl("/usr/bin/dde-session", "dde-session", "--systemd-service", nullptr);
qCritical() << "Failed to exec dde-session:" << strerror(errno);
_exit(1);
}
return pid;
}
static pid_t forkAdjuster(pid_t ddeSessionPid, int targetScore)
{
pid_t pid = fork();
if (pid < 0) {
qCritical() << "Failed to fork adjuster:" << strerror(errno);
return -1;
}
if (pid == 0) {
if (setpgid(0, 0) < 0) {
qCritical() << "Adjuster: setpgid failed:" << strerror(errno);
_exit(1);
}
if (!writeOomScore(ddeSessionPid, targetScore)) {
_exit(1);
}
_exit(0);
}
return pid;
}
```
`main` then becomes mostly orchestration:
```c++
pid_t ddeSessionPid = startDdeSession();
if (ddeSessionPid < 0)
return 1;
// wait/check, print owner, etc.
const int targetScore = -999;
pid_t adjusterPid = forkAdjuster(ddeSessionPid, targetScore);
if (adjusterPid < 0) {
kill(ddeSessionPid, SIGTERM);
return 1;
}
// waitpid(adjusterPid, &status, 0);
// verify result and tests
// wait for dde-session
```
This keeps all current behavior (dual fork, test logic, logging) but makes responsibilities and error paths much clearer.
---
### 3. Decouple test semantics from the launcher control flow
The “Test PASSED/FAILED” plus hardcoded `-500` expectation are test semantics mixed into the launcher path. You can isolate them into a dedicated function, keeping current behavior but making the main path easier to scan:
```c++
static void verifyAdjusterResult(pid_t ddeSessionPid, int expectedScore)
{
const int newValue = readOomScore(ddeSessionPid);
if (newValue == kOomReadError) {
qWarning() << "✗ Test FAILED: Unable to read oom_score_adj";
return;
}
qInfo() << "dde-session's new oom_score_adj:" << newValue;
if (newValue == expectedScore) {
qInfo() << "✓ Test PASSED";
} else {
qWarning() << "✗ Test FAILED: Expected" << expectedScore << ", got" << newValue;
}
}
```
Usage in `main` stays functionally identical:
```c++
if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
verifyAdjusterResult(ddeSessionPid, -500);
} else {
qWarning() << "✗ Test FAILED: Adjuster process failed";
}
```
---
### 4. Tighten logging to reduce noise
Without changing what is logged, you can collapse some step-by-step messages into more structured ones to reduce visual clutter:
```c++
qInfo() << "=== DDE Session Launcher ===";
qInfo() << "Launching dde-session and adjusting oom_score_adj...";
qInfo() << "dde-session PID:" << ddeSessionPid << "- waiting for initialization...";
```
This preserves all key information while making the main algorithm easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| int newValue = readOomScore(ddeSessionPid); | ||
| if (newValue != -9999) { | ||
| qInfo() << "dde-session's new oom_score_adj:" << newValue; | ||
| if (newValue == -500) { |
There was a problem hiding this comment.
issue (bug_risk): The verification expects -500 but the adjuster writes -999, so the check will always fail on success.
The adjuster writes targetScore = -999 to oom_score_adj, but the verification checks newValue == -500. On success newValue will be -999, so this code will always treat a correct write as a failure. Align the written value and the expected value in the check so this path reports the outcome correctly.
src/dde-session-launcher/README.md
Outdated
| ### 方式1：直接运行（不会继承 file capabilities） | ||
|
|
||
| ```bash | ||
| ./build/src/dde-session-launcher/dde-session-launcher |
There was a problem hiding this comment.
issue (bug_risk): The example run path is inconsistent with the earlier build steps and likely incorrect.
Since the build instructions cd build, the binary path from there should be ./src/dde-session-launcher/dde-session-launcher. Using ./build/src/... would resolve to build/build/... and likely fail. Please update this (and similar examples) to match the actual working directory after the build step.
| } | ||
| } | ||
|
|
||
| int main(int argc, char *argv[]) |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the launcher by extracting helpers, centralizing oom_score_adj handling, and simplifying logging to keep behavior while making the control flow easier to follow.
You can keep all current behavior but significantly cut complexity with a few focused refactors:
1. Remove magic sentinel and unify /proc path construction
Right now -9999 is used as an error sentinel and /proc/%d/oom_score_adj is copy-pasted in three places. You can centralize this without changing behavior:
namespace {
constexpr int kOomReadError = std::numeric_limits<int>::min();
inline QString oomScoreAdjPath(pid_t pid)
{
return QString("/proc/%1/oom_score_adj").arg(pid);
}
} // namespace
static int readOomScore(pid_t pid)
{
const QByteArray path = oomScoreAdjPath(pid).toLocal8Bit();
char buf[32];
int fd = open(path.constData(), O_RDONLY);
if (fd < 0) {
return kOomReadError;
}
ssize_t n = read(fd, buf, sizeof(buf) - 1);
close(fd);
if (n <= 0) {
return kOomReadError;
}
buf[n] = '\0';
return atoi(buf);
}
static bool writeOomScore(pid_t pid, int oomScore)
{
const QByteArray path = oomScoreAdjPath(pid).toLocal8Bit();
char buf[32];
int fd = open(path.constData(), O_WRONLY);
// ...
}Then replace "/proc/%d/oom_score_adj" usages and -9999 checks with oomScoreAdjPath() and kOomReadError.
2. Extract small helpers from main to reduce linear complexity
main mixes process management, oom operations, and test reporting. Pulling out small functions keeps the control flow obvious while preserving the current forking model:
static pid_t startDdeSession()
{
pid_t pid = fork();
if (pid < 0) {
qCritical() << "Failed to fork:" << strerror(errno);
return -1;
}
if (pid == 0) {
execl("/usr/bin/dde-session", "dde-session", "--systemd-service", nullptr);
qCritical() << "Failed to exec dde-session:" << strerror(errno);
_exit(1);
}
return pid;
}
static pid_t forkAdjuster(pid_t ddeSessionPid, int targetScore)
{
pid_t pid = fork();
if (pid < 0) {
qCritical() << "Failed to fork adjuster:" << strerror(errno);
return -1;
}
if (pid == 0) {
if (setpgid(0, 0) < 0) {
qCritical() << "Adjuster: setpgid failed:" << strerror(errno);
_exit(1);
}
if (!writeOomScore(ddeSessionPid, targetScore)) {
_exit(1);
}
_exit(0);
}
return pid;
}main then becomes mostly orchestration:
pid_t ddeSessionPid = startDdeSession();
if (ddeSessionPid < 0)
return 1;
// wait/check, print owner, etc.
const int targetScore = -999;
pid_t adjusterPid = forkAdjuster(ddeSessionPid, targetScore);
if (adjusterPid < 0) {
kill(ddeSessionPid, SIGTERM);
return 1;
}
// waitpid(adjusterPid, &status, 0);
// verify result and tests
// wait for dde-sessionThis keeps all current behavior (dual fork, test logic, logging) but makes responsibilities and error paths much clearer.
3. Decouple test semantics from the launcher control flow
The “Test PASSED/FAILED” plus hardcoded -500 expectation are test semantics mixed into the launcher path. You can isolate them into a dedicated function, keeping current behavior but making the main path easier to scan:
static void verifyAdjusterResult(pid_t ddeSessionPid, int expectedScore)
{
const int newValue = readOomScore(ddeSessionPid);
if (newValue == kOomReadError) {
qWarning() << "✗ Test FAILED: Unable to read oom_score_adj";
return;
}
qInfo() << "dde-session's new oom_score_adj:" << newValue;
if (newValue == expectedScore) {
qInfo() << "✓ Test PASSED";
} else {
qWarning() << "✗ Test FAILED: Expected" << expectedScore << ", got" << newValue;
}
}Usage in main stays functionally identical:
if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
verifyAdjusterResult(ddeSessionPid, -500);
} else {
qWarning() << "✗ Test FAILED: Adjuster process failed";
}4. Tighten logging to reduce noise
Without changing what is logged, you can collapse some step-by-step messages into more structured ones to reduce visual clutter:
qInfo() << "=== DDE Session Launcher ===";
qInfo() << "Launching dde-session and adjusting oom_score_adj...";
qInfo() << "dde-session PID:" << ddeSessionPid << "- waiting for initialization...";This preserves all key information while making the main algorithm easier to follow.
fly602
force-pushed
the
master
branch
3 times, most recently
from
a0eddd0 to
90df134
Compare
fly602
force-pushed
the
master
branch
3 times, most recently
from
7af44a4 to
aab6416
Compare
fly602
changed the title
debian/postinst
Outdated
| # | ||
| # see: dh_installdeb(1) | ||
|
|
||
| set -e |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new tool dde-oom-score-adj to protect critical DDE services from being terminated by the Linux OOM (Out-Of-Memory) killer by adjusting their OOM scores. The tool monitors systemd services and automatically adjusts their OOM scores to -999, making them less likely to be killed during low-memory situations.
Key Changes:
- Implements a Qt-based daemon that monitors systemd service startup via D-Bus
- Adds DConfig-based configuration for customizable service monitoring lists
- Includes automatic capability management via Debian postinst script
- Tool automatically exits after 3 minutes or when all monitored services are adjusted
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/dde-oom-score-adj/oomscoreadjuster.h | Header file defining the OomScoreAdjuster class with systemd monitoring and OOM score adjustment capabilities |
| tools/dde-oom-score-adj/oomscoreadjuster.cpp | Main implementation with D-Bus integration, systemd signal monitoring, and /proc filesystem manipulation |
| tools/dde-oom-score-adj/main.cpp | Application entry point with capability checking and application lifecycle management |
| tools/dde-oom-score-adj/dde-oom-score-adj.desktop | XDG autostart desktop entry for automatic execution during system startup |
| tools/dde-oom-score-adj/CMakeLists.txt | Build configuration for the new tool with Qt, DTK, and libcap-ng dependencies |
| tools/CMakeLists.txt | Added subdirectory reference to build the new tool |
| misc/dconf/org.deepin.dde.session.oom-score-adj.json | DConfig schema defining the monitored services list configuration |
| misc/CMakeLists.txt | Installation rules for the DConfig configuration file |
| debian/postinst | Post-installation script to set CAP_SYS_RESOURCE capability on the binary |
| debian/control | Added libcap-ng-dev build dependency |
| REUSE.toml | Updated license annotations to include desktop files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| delete config; | ||
| } | ||
| qCritical() << "Failed to load DConfig configuration, exiting."; | ||
| exit(-1); |
There was a problem hiding this comment.
The call to exit(-1) terminates the entire process abruptly without proper cleanup. This prevents Qt's event loop cleanup, destructor calls, and signal/slot disconnections. This should emit the timeoutExit() signal to allow graceful application shutdown via QCoreApplication::quit(), or at minimum use QCoreApplication::exit() with a non-zero exit code.
| exit(-1); | |
| emit timeoutExit(); | |
| return; |
| const QDBusArgument arg = reply.arguments().at(0).value<QDBusArgument>(); | ||
|
|
||
| arg.beginArray(); | ||
| pids.reserve(32); // 预分配空间，减少内存重新分配 |
There was a problem hiding this comment.
The reserve(32) call reserves space for 32 PIDs, but this seems arbitrary. A service typically has far fewer processes. A more reasonable default would be 4-8 processes, or you could base it on the actual number of services being monitored. Over-reserving wastes memory, while this specific number doesn't relate to any documented service behavior.
| pids.reserve(32); // 预分配空间，减少内存重新分配 |
| Comment=Adjust OOM score for critical DDE services | ||
| Exec=/usr/bin/dde-oom-score-adj | ||
| Terminal=false | ||
| NoDisplay=true |
There was a problem hiding this comment.
The desktop file is missing the X-GNOME-Autostart-enabled field which is commonly used to control autostart behavior. Additionally, for proper XDG autostart specification compliance, consider adding OnlyShowIn or NotShowIn fields to ensure this only runs in appropriate desktop environments. Since this is DDE-specific, adding "OnlyShowIn=Deepin;" would be appropriate.
| NoDisplay=true | |
| NoDisplay=true | |
| OnlyShowIn=Deepin; | |
| X-GNOME-Autostart-enabled=true |
| # This allows it to adjust oom_score_adj for monitored services | ||
| if [ -x /usr/bin/dde-oom-score-adj ]; then | ||
| setcap cap_sys_resource=ep /usr/bin/dde-oom-score-adj || true | ||
| fi |
There was a problem hiding this comment.
The postinst script is missing the debhelper token "#DEBHELPER#" which is standard practice in Debian packaging. This token is replaced by debhelper with any generated maintainer script fragments from the package build. Without it, any auto-generated scripts from debhelper won't be included, potentially causing incomplete package configuration.
| fi | |
| fi | |
| #DEBHELPER# |
| # Set CAP_SYS_RESOURCE capability on dde-oom-score-adj | ||
| # This allows it to adjust oom_score_adj for monitored services | ||
| if [ -x /usr/bin/dde-oom-score-adj ]; then | ||
| setcap cap_sys_resource=ep /usr/bin/dde-oom-score-adj || true |
There was a problem hiding this comment.
The postinst script uses "|| true" which silently suppresses all errors from setcap, including permission errors, missing binary, or invalid capability names. This makes debugging difficult if the capability setting fails. Consider logging the failure or at minimum documenting why failures are acceptable. A better approach would be to check the return value and log a warning to help system administrators identify configuration issues.
| setcap cap_sys_resource=ep /usr/bin/dde-oom-score-adj || true | |
| if command -v setcap >/dev/null 2>&1; then | |
| if ! setcap cap_sys_resource=ep /usr/bin/dde-oom-score-adj; then | |
| echo "warning: failed to set CAP_SYS_RESOURCE on /usr/bin/dde-oom-score-adj" >&2 | |
| fi | |
| else | |
| echo "warning: setcap not found; cannot set CAP_SYS_RESOURCE on /usr/bin/dde-oom-score-adj" >&2 | |
| fi |
| } | ||
|
|
||
| bool OomScoreAdjuster::adjustOomScore(quint32 pid, int score) | ||
| { |
There was a problem hiding this comment.
The QString::arg() method is used to format the path, but no validation is performed on the pid value. While quint32 ensures it's positive, extremely large or invalid PID values could still be passed through. The /proc filesystem will naturally fail to open for invalid PIDs, which is handled by the file.open() check, but it would be more defensive to validate that the PID is reasonable (e.g., not 0, and within system limits) before attempting file operations.
| { | |
| { | |
| // Validate PID before using it to build /proc path | |
| if (pid == 0) { | |
| return false; | |
| } | |
| long maxPid = sysconf(_SC_PID_MAX); | |
| if (maxPid > 0 && pid > static_cast<quint32>(maxPid)) { | |
| return false; | |
| } |
tools/dde-oom-score-adj/main.cpp
Outdated
| // 在新线程中启动 | ||
| QThread::create([&adjuster]() { | ||
| QMetaObject::invokeMethod(&adjuster, "start", Qt::QueuedConnection); | ||
| })->start(); |
There was a problem hiding this comment.
The thread creation pattern here is problematic. QThread::create() creates a new thread, but the lambda function is executed in that thread, while invokeMethod with Qt::QueuedConnection will queue the call on the adjuster's thread (which is the main thread). This means:
- A new thread is created but serves no real purpose
- The thread is never properly managed or cleaned up
- The start() method will still execute in the main thread
If the intention is to execute start() asynchronously in the main thread, you should simply call it directly with QMetaObject::invokeMethod without creating a thread. If the intention is to run in a separate thread, the adjuster object should be moved to that thread properly using QObject::moveToThread().
| // 在新线程中启动 | |
| QThread::create([&adjuster]() { | |
| QMetaObject::invokeMethod(&adjuster, "start", Qt::QueuedConnection); | |
| })->start(); | |
| // 异步启动：在主线程事件循环中调用 start() | |
| QMetaObject::invokeMethod(&adjuster, "start", Qt::QueuedConnection); |
|
|
||
| delete config; | ||
| } else { | ||
| if (config) { | ||
| delete config; | ||
| } |
There was a problem hiding this comment.
Memory leak: The DConfig object is deleted with delete, but the QObject parent system is not properly utilized. Additionally, when config is invalid, it is deleted again (lines 55-56) which could lead to double-delete if the first branch (line 53) was taken. The pattern should either use proper parent-based lifetime management by passing 'this' as parent (which you do on line 42), or consistently handle deletion. Since you're already passing 'this' as parent, the manual delete calls are unnecessary and potentially dangerous as Qt will automatically delete the child when the parent is destroyed.
| delete config; | |
| } else { | |
| if (config) { | |
| delete config; | |
| } | |
| } else { |
| : QObject(parent) | ||
| , m_systemdBus(QDBusConnection::sessionBus()) | ||
| , m_exitTimer(new QTimer(this)) | ||
| , m_targetScore(-999) |
There was a problem hiding this comment.
The target OOM score is hardcoded to -999, but the PR description mentions that DConfig should allow customization of the "target score". The configuration file only defines monitorServices but doesn't include a configurable target score field. Either the target score should be made configurable through DConfig as the description suggests, or the PR description should be updated to clarify that only the service list is configurable.
| "serial": 0, | ||
| "flags": ["global"], | ||
| "name": "monitorServices", | ||
| "name[zh_CN]": "OOM 分数调整监控服务列表", |
There was a problem hiding this comment.
Inconsistent Chinese translation style: The name field uses English "OOM" abbreviation without explanation in "OOM 分数调整监控服务列表", which may be unclear to Chinese users who are not familiar with the technical term. Consider providing the full Chinese translation or adding a brief explanation, such as "内存溢出(OOM)分数调整监控服务列表" for better clarity.
| "name[zh_CN]": "OOM 分数调整监控服务列表", | |
| "name[zh_CN]": "内存溢出(OOM)分数调整监控服务列表", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 检查是否有 CAP_SYS_RESOURCE capability（仅用于调试信息） | ||
| if (!capng_have_capability(CAPNG_EFFECTIVE, CAP_SYS_RESOURCE)) { | ||
| qWarning() << "Warning: Missing CAP_SYS_RESOURCE capability!"; | ||
| qWarning() << "The program may not be able to modify oom_score_adj."; | ||
| qWarning() << "When running as systemd service, capabilities are granted automatically."; | ||
| } |
There was a problem hiding this comment.
The capability check in main.cpp only logs a warning when CAP_SYS_RESOURCE is missing, but the program continues to run. This could lead to silent failures where the tool runs for 3 minutes but never successfully adjusts any OOM scores. Consider exiting immediately with a non-zero exit code if the capability is missing and this is not running as a systemd service, to provide faster feedback about the misconfiguration.
| "name": "monitorServices", | ||
| "name[zh_CN]": "OOM 分数调整监控服务列表", | ||
| "description": "List of systemd services to monitor for OOM score adjustment", | ||
| "permissions": "readonly", |
There was a problem hiding this comment.
The permissions field is set to "readonly" which means users cannot modify the monitored services list through DConfig. However, the PR description mentions "customizable service monitoring list", which implies this should be configurable. If the intent is to make this configurable by administrators or users, change permissions to "readwrite". If it should only be modifiable by the system, consider using "readwrite" with appropriate visibility settings or clarify in the description that it's readonly by design.
| "permissions": "readonly", | |
| "permissions": "readwrite", |
| bool OomScoreAdjuster::adjustOomScore(quint32 pid, int score) | ||
| { | ||
| if (pid == 0) { | ||
| return false; | ||
| } | ||
|
|
||
| QString procPath = QString("/proc/%1/oom_score_adj").arg(pid); | ||
|
|
||
| QFile file(procPath); | ||
| if (!file.open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Unbuffered)) { | ||
| return false; | ||
| } | ||
|
|
||
| QTextStream out(&file); | ||
| out << score; | ||
| out.flush(); | ||
| file.close(); |
There was a problem hiding this comment.
The code doesn't verify that the score value written is within the valid range for oom_score_adj (-1000 to 1000). While -999 is valid, if the code is later modified to read the target score from configuration, invalid values could be passed. Consider adding a range check to ensure the score is between -1000 and 1000 before attempting to write it.
| void OomScoreAdjuster::subscribeToSystemd() | ||
| { | ||
| // 连接到 systemd 的 UnitNew 信号 | ||
| m_systemdBus.connect( | ||
| "org.freedesktop.systemd1", | ||
| "/org/freedesktop/systemd1", | ||
| "org.freedesktop.systemd1.Manager", | ||
| "UnitNew", | ||
| this, | ||
| SLOT(onUnitNew(QString, QDBusObjectPath)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The subscribeToSystemd method only subscribes to the UnitNew signal but doesn't subscribe to PropertiesChanged for existing services that are found by checkExistingServices. This means if an existing service restarts or changes state, the tool won't receive notifications about it. Consider also subscribing to PropertiesChanged for all services found during checkExistingServices, similar to what's done in onUnitNew.
|
|
||
| void OomScoreAdjuster::checkExistingServices() | ||
| { | ||
| for (const QString &serviceName : m_watchedServices) { |
There was a problem hiding this comment.
When checkExistingServices is called, it iterates through all watched services and calls checkAndAdjustService for each. However, if getUnitObjectPath fails for a service (returns empty string), the service path won't be cached in m_servicePathMap. This means the PropertiesChanged signal subscription in onUnitNew will never be set up for that service, even if it starts later. Consider caching the object path in checkAndAdjustService when getServiceProcesses successfully retrieves it, or setting up the PropertiesChanged subscription in checkExistingServices for services that exist.
| for (const QString &serviceName : m_watchedServices) { | |
| for (const QString &serviceName : m_watchedServices) { | |
| // 如果还没有缓存该服务的 unit 路径，尝试获取并订阅属性变化 | |
| if (!m_servicePathMap.contains(serviceName)) { | |
| const QString unitPath = getUnitObjectPath(serviceName); | |
| if (!unitPath.isEmpty()) { | |
| m_servicePathMap[serviceName] = unitPath; | |
| // 监听该 unit 的属性变化（与 onUnitNew 中的逻辑保持一致） | |
| m_systemdBus.connect( | |
| "org.freedesktop.systemd1", | |
| unitPath, | |
| "org.freedesktop.DBus.Properties", | |
| "PropertiesChanged", | |
| this, | |
| SLOT(onPropertiesChanged(QString, QVariantMap, QStringList)) | |
| ); | |
| } | |
| } |
|
|
||
| // 如果已经是目标值，跳过 | ||
| if (currentScore.toInt() == m_targetScore) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The code reads the current oom_score_adj value and skips adjustment if it matches the target score. However, if the file read fails (file.open returns false), the code falls through and attempts adjustment anyway. This could lead to unnecessary write attempts and log messages. Consider explicitly handling the read failure case, or at least logging when the current value cannot be determined.
| // 如果已经是目标值，跳过 | |
| if (currentScore.toInt() == m_targetScore) { | |
| continue; | |
| } | |
| } | |
| // 如果已经是目标值，跳过 | |
| if (currentScore.toInt() == m_targetScore) { | |
| continue; | |
| } | |
| } else { | |
| qWarning() << "Failed to read current OOM score for" << serviceName << "PID:" << pid; | |
| allAdjusted = false; | |
| continue; | |
| } |
| void OomScoreAdjuster::loadServiceList() | ||
| { | ||
| // 从 DConfig 读取服务列表 | ||
| // 注意：传入 this 作为 parent，Qt 会自动管理内存，不需要手动 delete | ||
| DConfig *config = DConfig::create("org.deepin.dde.session", "org.deepin.dde.session.oom-score-adj", QString(), this); |
There was a problem hiding this comment.
The comment says "传入 this 作为 parent，Qt 会自动管理内存，不需要手动 delete" (Passing this as parent, Qt will automatically manage memory, no need to manually delete). However, the loaded DConfig object is never stored or used beyond this function. This creates a potential issue: if DConfig initialization is asynchronous or if the config values could change at runtime, the object might be destroyed before it's fully initialized since it goes out of scope. Consider storing the DConfig pointer as a member variable to keep it alive and potentially react to configuration changes.
| const QDBusArgument arg = reply.arguments().at(0).value<QDBusArgument>(); | ||
|
|
||
| arg.beginArray(); | ||
| while (!arg.atEnd()) { | ||
| QString cgroupPath; | ||
| quint32 pid = 0; | ||
| QString command; | ||
|
|
||
| arg.beginStructure(); | ||
| arg >> cgroupPath >> pid >> command; | ||
| arg.endStructure(); | ||
|
|
||
| if (pid > 0) { | ||
| pids.append(pid); | ||
| } | ||
| } | ||
| arg.endArray(); |
There was a problem hiding this comment.
The QDBusArgument parsing code doesn't check if the argument type is actually an array before calling beginArray(). If the D-Bus API changes or returns an unexpected type, this could cause a crash or undefined behavior. Consider verifying the argument signature matches the expected "a(sus)" format before parsing.
| { | ||
| loadServiceList(); | ||
|
|
||
| // 设置3分钟后自动退出 |
There was a problem hiding this comment.
The comment on line 29 states "设置3分钟后自动退出" (Set to automatically exit after 3 minutes), but the timer is created in the constructor and never started there. The timer is only started in the start() method if not all services are adjusted immediately. This discrepancy between the comment and actual behavior could be confusing. Consider updating the comment to clarify that the timer is created here but started conditionally in start().
| // 设置3分钟后自动退出 | |
| // 配置 3 分钟超时定时器（在 start() 中按需启动，用于自动退出） |
| // 如果列表为空，使用默认服务 | ||
| if (m_watchedServices.isEmpty()) { | ||
| m_watchedServices << "dde-session-manager.service" << "dde-session@x11.service"; |
There was a problem hiding this comment.
这个要不要移出去，当做DConfig错误时候的fallback，
因为如果DConfig是有效的话，应该是能够读到的，我看那个配置是readonly的，只会被override，
misc/CMakeLists.txt
Outdated
| install(FILES ${PROFILE} DESTINATION /etc/profile.d) | ||
|
|
||
| # 安装 DConfig 配置文件 | ||
| install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/dconf/org.deepin.dde.session.oom-score-adj.json |
There was a problem hiding this comment.
可以使用dtkcore提供的cmake函数进行安装配置文件，类似https://github.com/linuxdeepin/dde-control-center/blob/76bd64c0ed51797ddab9992d3c050dd6a06d5ab4/CMakeLists.txt#L191
| * @brief The OomScoreAdjuster class | ||
| * @brief 监听 systemd 服务启动并调整其 OOM score | ||
| */ | ||
| class OomScoreAdjuster : public QObject, protected QDBusContext |
There was a problem hiding this comment.
这个需要对外提供dbus服务么？我好像没看到注册的地方，
|
|
||
| void OomScoreAdjuster::checkAndAdjustService(const QString &serviceName) | ||
| { | ||
| QList<quint32> pids = getServiceProcesses(serviceName); |
There was a problem hiding this comment.
这里这这个时机获取到的pids，是这个serviceName全部的进程么，会不会还有没有启动的，这样的话可能有漏掉的进程没有调整？
1. Implement dde-oom-score-adj tool to monitor and adjust OOM scores for critical DDE services 2. Add DConfig configuration for customizable service monitoring list and target score 3. Set up systemd signal monitoring to detect service startup and adjust OOM scores in real-time 4. Include automatic capability management via postinst script for CAP_SYS_RESOURCE 5. Add autostart desktop file for automatic execution during system startup 6. The tool automatically exits after 3 minutes to avoid long-running processes Log: Added OOM score adjustment feature to protect critical DDE services from being killed by the system OOM killer Influence: 1. Test that dde-oom-score-adj starts automatically during system boot 2. Verify critical DDE services (dde-session-manager, dde-session@x11) have adjusted OOM scores 3. Check that the tool exits after 3 minutes as expected 4. Test configuration changes through DConfig interface 5. Verify capability assignment works correctly during package installation 6. Monitor system logs for any OOM adjustment related errors feat: 为关键 DDE 服务添加 OOM 分数调整工具 1. 实现 dde-oom-score-adj 工具来监控和调整关键 DDE 服务的 OOM 分数 2. 添加 DConfig 配置用于可自定义的服务监控列表和目标分数 3. 设置 systemd 信号监控以实时检测服务启动并调整 OOM 分数 4. 包含通过 postinst 脚本自动管理 CAP_SYS_RESOURCE 能力 5. 添加自动启动桌面文件用于系统启动时自动执行 6. 工具在 3 分钟后自动退出，避免长时间运行的进程 Log: 新增 OOM 分数调整功能，保护关键 DDE 服务不被系统 OOM killer 终止 Influence: 1. 测试 dde-oom-score-adj 在系统启动时是否自动启动 2. 验证关键 DDE 服务（dde-session-manager, dde-session@x11）是否具有调整 后的 OOM 分数 3. 检查工具是否在 3 分钟后按预期退出 4. 通过 DConfig 接口测试配置更改 5. 验证包安装期间的能力分配是否正确工作 6. 监控系统日志中是否有 OOM 调整相关的错误
deepin pr auto review我来对这次代码变更进行详细的审查：
a) 错误处理： // 在 main.cpp 中，建议在检查 capability 失败时退出
if (!capng_have_capability(CAPNG_EFFECTIVE, CAP_SYS_RESOURCE)) {
qCritical() << "CAP_SYS_RESOURCE capability is required!";
return 1;
}b) 资源管理： // 在 OomScoreAdjuster::adjustOomScore 中，建议使用 RAII 方式处理文件
bool OomScoreAdjuster::adjustOomScore(quint32 pid, int score)
{
QString procPath = QString("/proc/%1/oom_score_adj").arg(pid);
QFile file(procPath);
if (!file.open(QIODevice::WriteOnly | QIODevice::Text | QIODevice::Unbuffered)) {
qWarning() << "Failed to open" << procPath << ":" << file.errorString();
return false;
}
QTextStream out(&file);
out << score;
out.flush();
if (file.error() != QFile::NoError) {
qWarning() << "Failed to write to" << procPath << ":" << file.errorString();
return false;
}
return true;
}c) 配置验证： // 在 loadServiceList 中添加配置验证
void OomScoreAdjuster::loadServiceList()
{
DConfig *config = DConfig::create("org.deepin.dde.session", "org.deepin.dde.session.oom-score-adj", QString(), this);
if (config && config->isValid()) {
QVariant value = config->value("monitorServices");
if (value.canConvert<QStringList>()) {
m_watchedServices = value.toStringList();
} else {
qWarning() << "Invalid monitorServices configuration format";
}
}
if (m_watchedServices.isEmpty()) {
qWarning() << "No valid services to monitor, using defaults";
m_watchedServices << "dde-session-manager.service" << "dde-session@x11.service";
}
}d) 安全性改进： // 在 checkAndAdjustService 中添加 PID 验证
void OomScoreAdjuster::checkAndAdjustService(const QString &serviceName)
{
QList<quint32> pids = getServiceProcesses(serviceName);
if (pids.isEmpty()) {
return;
}
for (quint32 pid : pids) {
// 验证 PID 是否属于当前用户的服务
if (pid == 0 || !isValidServicePid(pid, serviceName)) {
qWarning() << "Invalid PID" << pid << "for service" << serviceName;
continue;
}
if (adjustOomScore(pid, m_targetScore)) {
qInfo() << "Successfully adjusted OOM score for" << serviceName << "PID:" << pid;
}
}
}
a) 缓存优化： // 在 OomScoreAdjuster 类中添加缓存机制
private:
QCache<QString, QList<quint32>> m_pidCache; // 缓存服务 PID 列表
static const int CACHE_TIMEOUT = 30000; // 30秒缓存超时b) 异步处理： // 将耗时的 D-Bus 调用改为异步
void OomScoreAdjuster::checkServiceAsync(const QString &serviceName)
{
QDBusPendingCall async = m_systemdBus.asyncCall(
"GetUnit", serviceName
);
QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(async, this);
connect(watcher, &QDBusPendingCallWatcher::finished, [this, serviceName](QDBusPendingCallWatcher *watcher) {
QDBusPendingReply<QDBusObjectPath> reply = *watcher;
watcher->deleteLater();
if (reply.isValid()) {
handleServiceUnit(serviceName, reply.value());
}
});
}
a) 权限检查： // 在启动时检查必要的权限
bool OomScoreAdjuster::checkPermissions()
{
// 检查是否有权限访问 /proc
if (access("/proc", R_OK | W_OK) != 0) {
qCritical() << "No permission to access /proc";
return false;
}
// 检查是否有权限修改 oom_score_adj
QString testPath = QString("/proc/%1/oom_score_adj").arg(QCoreApplication::applicationPid());
if (access(testPath.toLocal8Bit(), W_OK) != 0) {
qCritical() << "No permission to modify oom_score_adj";
return false;
}
return true;
}b) 输入验证： // 在处理服务名时进行验证
bool OomScoreAdjuster::isValidServiceName(const QString &serviceName)
{
// 检查服务名格式
if (!serviceName.endsWith(".service")) {
return false;
}
// 检查是否包含非法字符
static QRegularExpression invalidChars("[^a-zA-Z0-9-@_.]");
if (serviceName.contains(invalidChars)) {
return false;
}
return true;
}
a) 日志系统： // 使用结构化日志
void OomScoreAdjuster::logServiceEvent(const QString &event, const QString &service, quint32 pid = 0)
{
QJsonObject logEntry;
logEntry["timestamp"] = QDateTime::currentDateTime().toString(Qt::ISODate);
logEntry["event"] = event;
logEntry["service"] = service;
if (pid > 0) {
logEntry["pid"] = static_cast<int>(pid);
}
qInfo().noquote() << QJsonDocument(logEntry).toJson(QJsonDocument::Compact);
}b) 配置热重载： // 添加配置热重载支持
void OomScoreAdjuster::setupConfigWatcher()
{
DConfig *config = DConfig::create("org.deepin.dde.session", "org.deepin.dde.session.oom-score-adj", QString(), this);
connect(config, &DConfig::valueChanged, [this](const QString &key) {
if (key == "monitorServices") {
loadServiceList();
checkExistingServices();
}
});
}这些改进建议主要关注：
建议在实现这些改进时，保持代码的可维护性和可读性，并确保不会引入新的安全风险。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, fly602 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 |
4 participants
critical DDE services
and target score
OOM scores in real-time
CAP_SYS_RESOURCE
startup
processes
Log: Added OOM score adjustment feature to protect critical DDE services
from being killed by the system OOM killer
Influence:
have adjusted OOM scores
installation
feat: 为关键 DDE 服务添加 OOM 分数调整工具
Log: 新增 OOM 分数调整功能，保护关键 DDE 服务不被系统 OOM killer 终止
Influence:
后的 OOM 分数