Fix: [custom] The memory size not show on special platform. #575
Conversation
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
Reviewer's GuideThis PR adds a thread-safe helper to detect the presence of a PCI graphics card via lspci and uses it to select the appropriate device generator on custom platforms and to conditionally pre-cache GPU info, fixing missing memory size reporting on certain special platforms. Sequence diagram for DeviceFactory::getDeviceGenerator with PCI graphics detectionsequenceDiagram
participant Client
participant DeviceFactory
participant CommonTools
participant HWGenerator
participant ArmGenerator
participant CustomGenerator
Client->>DeviceFactory: getDeviceGenerator()
activate DeviceFactory
alt type is HW
DeviceFactory->>HWGenerator: create
DeviceFactory-->>Client: HWGenerator instance
else type is PGUX
DeviceFactory->>HWGenerator: create
DeviceFactory-->>Client: HWGenerator instance
else type is CustomType
DeviceFactory->>CommonTools: hasPciGraphicsCard()
activate CommonTools
CommonTools->>CommonTools: run lspci via QProcess
CommonTools-->>DeviceFactory: bool hasPci
deactivate CommonTools
alt hasPci == true
DeviceFactory->>ArmGenerator: create
DeviceFactory-->>Client: ArmGenerator instance
else hasPci == false
DeviceFactory->>CustomGenerator: create
DeviceFactory-->>Client: CustomGenerator instance
end
else other type
DeviceFactory->>HWGenerator: create
DeviceFactory-->>Client: HWGenerator instance
end
deactivate DeviceFactory
Class diagram for CommonTools PCI graphics detection and DeviceFactory generator selectionclassDiagram
class CommonTools {
<<QObject>>
+static QString getGpuInfoCommandFromDConfig()
+static QString preGenerateGpuInfo()
+static bool hasPciGraphicsCard()
-static bool getGpuBaseInfo(QMap~QString,QString~ &mapInfo)
}
class DeviceFactory {
+static DeviceGenerator* getDeviceGenerator()
}
class DeviceGenerator {
}
class HWGenerator {
}
class ArmGenerator {
}
class CustomGenerator {
}
DeviceFactory ..> DeviceGenerator : creates
DeviceFactory ..> HWGenerator : may create
DeviceFactory ..> ArmGenerator : may create
DeviceFactory ..> CustomGenerator : may create
DeviceFactory ..> CommonTools : uses hasPciGraphicsCard
HWGenerator --|> DeviceGenerator
ArmGenerator --|> DeviceGenerator
CustomGenerator --|> DeviceGenerator
CommonTools ..> QProcess : uses
CommonTools ..> QMutex : uses
CommonTools ..> QMutexLocker : uses
Flow diagram for main startup GPU info pre-generation with PCI detectionflowchart TD
A[start_main] --> B[Check specialComType]
B -->|specialComType == kCustomType| C[Call CommonTools_hasPciGraphicsCard]
B -->|specialComType != kCustomType| E[Skip_preGenerateGpuInfo]
C --> D{hasPciGraphicsCard == false}
D -->|true| F[Call CommonTools_preGenerateGpuInfo]
D -->|false| E
F --> G[Continue_application_startup]
E --> G
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
hasPciGraphicsCard()implementation blocks synchronously for up to 5 seconds viaQProcess::waitForFinished, which may freeze the main/UI thread where it’s used (e.g., inmain.cppandDeviceFactory); consider either running this check off the UI thread or reducing/handling the timeout more defensively. - In
hasPciGraphicsCard(), the critical log message on non‑zero exit code has a typo (lspicinstead oflspci) and does not includereadAllStandardError(), which would make diagnosing failures easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `hasPciGraphicsCard()` implementation blocks synchronously for up to 5 seconds via `QProcess::waitForFinished`, which may freeze the main/UI thread where it’s used (e.g., in `main.cpp` and `DeviceFactory`); consider either running this check off the UI thread or reducing/handling the timeout more defensively.
- In `hasPciGraphicsCard()`, the critical log message on non‑zero exit code has a typo (`lspic` instead of `lspci`) and does not include `readAllStandardError()`, which would make diagnosing failures easier.
## Individual Comments
### Comment 1
<location> `deepin-devicemanager/src/Tool/commontools.cpp:309-318` </location>
<code_context>
return (gpuBaseInfo + gpuMemInfo);
}
+bool CommonTools::hasPciGraphicsCard()
+{
+ static bool result = false;
+ static bool initialized = false;
+ static QMutex mutex;
+
+ QMutexLocker locker(&mutex);
+
+ if (!initialized) {
+ QProcess process;
+ process.setProgram("lspci");
+ process.setArguments(QStringList() << "-v" << "-d" << "::0300");
+ process.start();
+ if (!process.waitForFinished(5000)) {
+ qCritical() << "Exec lspci time out!";
+ result = false;
</code_context>
<issue_to_address>
**suggestion:** Handle QProcess start failures and distinguish them from timeouts/normal non-zero exits.
Right now we only check `waitForFinished(5000)` and `exitCode()`. If `lspci` fails to start (missing binary, permissions, etc.), `waitForFinished` may still return `true`, and `exitCode()` can be misleading/undefined. Please handle `process.start()`/`waitForStarted()` and inspect `process.error()` (e.g. treat `QProcess::FailedToStart` as a hard failure and log it) so start failures are distinguished from timeouts and normal non-zero exits.
Suggested implementation:
```cpp
bool CommonTools::hasPciGraphicsCard()
{
static bool result = false;
static bool initialized = false;
static QMutex mutex;
QMutexLocker locker(&mutex);
if (!initialized) {
QProcess process;
process.setProgram("lspci");
process.setArguments(QStringList() << "-v" << "-d" << "::0300");
process.start();
// Distinguish start failures from timeouts and normal exits
if (!process.waitForStarted(1000)) {
if (process.error() == QProcess::FailedToStart) {
qCritical() << "Failed to start lspci:" << process.errorString();
} else {
qCritical() << "Starting lspci timed out or failed:" << process.errorString();
}
result = false;
} else if (!process.waitForFinished(5000)) {
qCritical() << "Exec lspci time out!";
result = false;
} else if (process.exitStatus() != QProcess::NormalExit) {
qCritical() << "Exec lspci crashed or exited abnormally, status:" << process.exitStatus()
<< ", error:" << process.errorString();
result = false;
} else if (process.exitCode() != 0) {
qCritical() << "Failed to exec lspci, code:" << process.exitCode();
result = false;
} else {
QString output = process.readAllStandardOutput();
result = !output.trimmed().isEmpty();
}
initialized = true;
}
```
1. Ensure the `hasPciGraphicsCard()` function ends with `return result;` (not visible in the snippet). If it does not, add `return result;` at the end of the function.
2. If the project’s logging style prefers `qWarning()` for some non-fatal conditions (e.g. timeouts vs. hard failures), you may want to adjust the `qCritical()` calls accordingly to match existing conventions in the rest of `commontools.cpp`.
</issue_to_address>
### Comment 2
<location> `deepin-devicemanager/src/Tool/commontools.cpp:325-327` </location>
<code_context>
+ if (!process.waitForFinished(5000)) {
+ qCritical() << "Exec lspci time out!";
+ result = false;
+ } else if (process.exitCode() != 0) {
+ qCritical() << "Failed to exec lspic, code : " << process.exitCode();
+ result = false;
+ } else {
</code_context>
<issue_to_address>
**nitpick (typo):** Fix the `lspci` log message typo to aid debugging.
The log currently says `"Failed to exec lspic"`, which doesn’t match the `lspci` command being run and can hinder log searches. Please correct the message to use `lspci`.
```suggestion
} else if (process.exitCode() != 0) {
qCritical() << "Failed to exec lspci, code : " << process.exitCode();
result = false;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lzwind
approved these changes
Dec 1, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GongHeng2017, lzwind 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 |
-- Add to to adjust the special platform to show memory size. Log: fix issue Bug: https://pms.uniontech.com/bug-view-342533.html
GongHeng2017
force-pushed
the
202512011123-eagle-fix
branch
from
618686d to
26a36d4
Compare
deepin pr auto review我来对这段代码进行审查和分析：
具体改进建议：
} else if (process.exitCode() != 0) {
QString error = process.readAllStandardError();
qCritical() << "Failed to exec lspci, code : " << process.exitCode()
<< "error: " << error;
result = false;
}
QProcess which;
which.setProgram("which");
which.setArguments(QStringList() << "lspci");
which.start();
if (!which.waitForFinished(1000) || which.exitCode() != 0) {
qCritical() << "lspci command not found";
return false;
}
static QReadWriteLock lock;
QReadLocker readLocker(&lock);
if (initialized) {
return result;
}
readLocker.unlock();
QWriteLocker writeLocker(&lock);
if (!initialized) { // 双重检查
// ... 执行检测逻辑
}
else if (type == "CustomType") {
// 对于定制类型，需要检查是否为PCI显卡平台
// 如果是PCI显卡平台，使用ARM生成器；否则使用自定义生成器
if (CommonTools::hasPciGraphicsCard()) {
generator = new ArmGenerator();
} else {
generator = new CustomGenerator();
}
}这些改进将使代码更加健壮、安全和高效。 |
Contributor
Author
|
/merge |
Contributor
|
This pr cannot be merged! (status: unstable) |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
deepin-bot
bot
merged commit c444291
into
linuxdeepin:develop/eagle
16 of 18 checks passed
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.
-- Add to to adjust the special platform to show memory size.
Log: fix issue
Bug: https://pms.uniontech.com/bug-view-342533.html
Summary by Sourcery
Adjust device generator selection and GPU info pre-caching on custom platforms based on the presence of a PCI graphics card.
Bug Fixes:
Enhancements: