Conversation
1. Root cause: isDpkgLocked() scanned ps output for "dpkg" processes and only excluded dpkg-query, causing false positives from other dpkg-related processes like dpkg-stat 2. Fix: replace ps scanning with non-blocking exclusive flock on dpkg lock files (/var/lib/dpkg/lock-frontend and /var/lib/dpkg/lock) to accurately detect active dpkg operations 3. Impact: isDpkgLocked() now returns accurate results, eliminating false positives and improving performance by avoiding QProcess Influence: 1. Test isDpkgLocked() returns false when no dpkg operation is running 2. Test isDpkgLocked() returns true when dpkg is actively running 3. Verify no regression in driver install/uninstall workflows fix: 使用flock检查dpkg锁替代ps进程扫描 1. 根因：isDpkgLocked()通过扫描ps输出中包含"dpkg"的进程来判断锁状态， 仅排除dpkg-query，其他dpkg相关进程如dpkg-stat会导致误报 2. 方案：使用非阻塞排他flock检查dpkg锁文件（/var/lib/dpkg/lock-frontend 和/var/lib/dpkg/lock）替代ps扫描，准确检测dpkg操作状态 3. 影响：isDpkgLocked()返回结果更准确，消除误报，避免QProcess开销提升性能 Influence: 1. 测试无dpkg操作运行时isDpkgLocked()返回false 2. 测试dpkg正在运行时isDpkgLocked()返回true 3. 验证驱动安装/卸载流程无回归 PMS: DEFECT-002
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: add-uos 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 guide (collapsed on small PRs)Reviewer's GuideUpdates isDpkgLocked() to determine whether dpkg is active by attempting non-blocking exclusive locks on the frontend and general dpkg lock files, eliminating false positives from unrelated dpkg-named processes and avoiding QProcess overhead. Sequence diagram for dpkg lock detectionsequenceDiagram
participant Caller
participant Utils
participant DpkgLockFiles
Caller->>Utils: isDpkgLocked()
loop each lock file
Utils->>DpkgLockFiles: open(lockFile, O_RDONLY | O_CLOEXEC)
alt open fails
Utils-->>Caller: true
else file opened
Utils->>DpkgLockFiles: flock(fd, LOCK_EX | LOCK_NB)
alt lock unavailable
Utils-->>Caller: true
else lock acquired
Utils->>DpkgLockFiles: close(fd)
end
end
end
Utils-->>Caller: false
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 - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="deepin-devicemanager-server/deepin-devicecontrol/src/drivercontrol/utils.cpp" line_range="243" />
<code_context>
+ "/var/lib/dpkg/lock"
+ };
+ for (const QString &lockFile : lockFiles) {
+ int fd = open(lockFile.toLocal8Bit().constData(), O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ return true;
+ }
+ if (flock(fd, LOCK_EX | LOCK_NB) == -1) {
+ close(fd);
+ return true;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** `flock(fd, LOCK_EX | LOCK_NB)` fails with `EBADF` because each lock file is opened with `O_RDONLY`, while an exclusive flock requires a file descriptor opened for writing. Since every existing dpkg lock file follows this path, `isDpkgLocked()` returns true even when no dpkg operation is running, causing installation workflows to retry until timeout.
**Triggers:** When either dpkg lock file exists, including the normal idle system state.
**Suggested fix:** Open the lock files with a writable mode such as `O_RDWR` before attempting `LOCK_EX`, or use a lock-checking operation compatible with the descriptor mode.
```suggestion
int fd = open(lockFile.toLocal8Bit().constData(), O_RDWR | O_CLOEXEC);
```
</issue_to_address>| "/var/lib/dpkg/lock" | ||
| }; | ||
| for (const QString &lockFile : lockFiles) { | ||
| int fd = open(lockFile.toLocal8Bit().constData(), O_RDONLY | O_CLOEXEC); |
There was a problem hiding this comment.
issue (bug_risk): flock(fd, LOCK_EX | LOCK_NB) fails with EBADF because each lock file is opened with O_RDONLY, while an exclusive flock requires a file descriptor opened for writing. Since every existing dpkg lock file follows this path, isDpkgLocked() returns true even when no dpkg operation is running, causing installation workflows to retry until timeout.
Triggers: When either dpkg lock file exists, including the normal idle system state.
Suggested fix: Open the lock files with a writable mode such as O_RDWR before attempting LOCK_EX, or use a lock-checking operation compatible with the descriptor mode.
Suggested change
| int fd = open(lockFile.toLocal8Bit().constData(), O_RDONLY | O_CLOEXEC); | |
| int fd = open(lockFile.toLocal8Bit().constData(), O_RDWR | O_CLOEXEC); |
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 语法正确，逻辑清晰。建议在 open() 失败时检查 errno，仅在文件确实被锁定时返回 true，其他错误（如 ENOENT）可记录日志并返回 false 2. 代码质量 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 代码结构清晰，注释完整。建议添加函数文档注释和错误日志，保持与同文件其他函数的风格一致性 3. 代码性能 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 性能良好，资源使用合理。本次修改将 QProcess 进程创建替换为直接系统调用，显著提升了性能。两次 open/flock/close 操作性能影响可忽略 4. 代码安全 🔒评价： 优秀 ✅ 通过
安全漏洞详情： 建议： 安全合规。本次修改移除了 QProcess 进程调用，降低了命令注入风险。使用 O_CLOEXEC 标志防止文件描述符泄漏到子进程，文件路径为硬编码常量，无路径遍历风险 💡 改进建议代码示例// 改进建议：增加 errno 检查和错误日志
bool Utils::isDpkgLocked()
{
// Check dpkg lock files using non-blocking exclusive flock.
const QStringList lockFiles = {
"/var/lib/dpkg/lock-frontend",
"/var/lib/dpkg/lock"
};
for (const QString &lockFile : lockFiles) {
int fd = open(lockFile.toLocal8Bit().constData(), O_RDONLY | O_CLOEXEC);
if (fd < 0) {
qCWarning(appLog) << "Failed to open lock file:" << lockFile
<< "errno:" << errno;
return true; // Conservative: assume locked
}
if (flock(fd, LOCK_EX | LOCK_NB) == -1) {
qCInfo(appLog) << "dpkg lock file is held:" << lockFile;
close(fd);
return true;
}
close(fd);
}
return false;
}本报告由 AI 代码审查工具自动生成 |
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
2 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.
fix: use flock to check dpkg lock instead of ps scan
and only excluded dpkg-query, causing false positives from other
dpkg-related processes like dpkg-stat
lock files (/var/lib/dpkg/lock-frontend and /var/lib/dpkg/lock)
to accurately detect active dpkg operations
false positives and improving performance by avoiding QProcess
Influence:
fix: 使用flock检查dpkg锁替代ps进程扫描
仅排除dpkg-query，其他dpkg相关进程如dpkg-stat会导致误报
和/var/lib/dpkg/lock）替代ps扫描，准确检测dpkg操作状态
Influence:
PMS: DEFECT-002
Summary by Sourcery
Replace process scanning with non-blocking dpkg lock checks to accurately determine whether package operations are active.
Bug Fixes:
Enhancements: