fix: Fix fail to verify bad sectors#176
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom Sep 22, 2025
Merged
Conversation
Reviewer's GuideThis PR introduces a new overloaded Utils::executCmd that separately captures stdout and stderr, updates its declaration, and refactors badblocks invocations in disk operation threads to use stderr output for verifying bad sectors instead of stdout. 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:
- Consolidate the two executCmd overloads (e.g. via default arguments or a single return struct) to avoid code duplication and simplify maintenance.
- Use nullptr instead of NULL when passing optional stdOut/stdErr pointers to executCmd for modern C++ style consistency.
- Rename the 'error' out-parameter to something more descriptive (e.g. 'procErrorString') to clearly differentiate it from standard error output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consolidate the two executCmd overloads (e.g. via default arguments or a single return struct) to avoid code duplication and simplify maintenance.
- Use nullptr instead of NULL when passing optional stdOut/stdErr pointers to executCmd for modern C++ style consistency.
- Rename the 'error' out-parameter to something more descriptive (e.g. 'procErrorString') to clearly differentiate it from standard error output.
## Individual Comments
### Comment 1
<location> `basestruct/utils.cpp:132-135` </location>
<code_context>
+ if (stdOut)
+ *stdOut = proc.readAllStandardOutput().data();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Direct conversion from QByteArray to QString may cause encoding issues.
Use QString::fromLocal8Bit or QString::fromUtf8 for conversion to handle non-ASCII output correctly, based on the expected encoding.
```suggestion
if (stdOut)
*stdOut = QString::fromLocal8Bit(proc.readAllStandardOutput());
if (stdErr)
*stdErr = QString::fromLocal8Bit(proc.readAllStandardError());
```
</issue_to_address>
### Comment 2
<location> `basestruct/utils.cpp:115` </location>
<code_context>
return exitcode;
}
+int Utils::executCmd(const QString &strCmd, QString *stdOut, QString *stdErr, QString &error)
+{
+ qDebug() << "Utils::executCmd" << strCmd;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring QProcess setup and teardown into a shared helper to eliminate duplication across overloads.
Consider pulling the QProcess‐setup and teardown into one helper, then have both overloads call it. For example:
```cpp
// private helper
static int runProcess(const QString &prog,
const QStringList &args,
QString *stdOut,
QString *stdErr,
QString &error)
{
QProcess proc;
proc.setProgram(prog);
proc.setArguments(args);
proc.start(QIODevice::ReadWrite);
proc.waitForFinished(-1);
if (stdOut) *stdOut = proc.readAllStandardOutput();
if (stdErr) *stdErr = proc.readAllStandardError();
error = proc.errorString();
int exitCode = proc.exitCode();
proc.close();
qDebug() << "runProcess exitCode:" << exitCode;
return exitCode;
}
```
Then reduce your two overloads to:
```cpp
int Utils::executCmd(const QString &strCmd,
QString &error)
{
auto args = parseCombinedArgString(strCmd);
if (args.isEmpty()) { error = "args is empty"; return -1; }
return runProcess(args.takeFirst(), args, nullptr, nullptr, error);
}
int Utils::executCmd(const QString &strCmd,
QString *stdOut,
QString *stdErr,
QString &error)
{
auto args = parseCombinedArgString(strCmd);
if (args.isEmpty()) { error = "args is empty"; return -1; }
return runProcess(args.takeFirst(), args, stdOut, stdErr, error);
}
```
This removes all duplicated parsing, QProcess setup, and logging while preserving both APIs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
In the previous version, the standard output of badblocks was mistakenly used for judgment. The new version uses the standard error output. Log: Fix fail to verify bad sectors Bug: https://pms.uniontech.com/bug-view-315835.html
wangrong1069
force-pushed
the
pr0919-2
branch
from
73bea3f to
d3cc43c
Compare
deepin pr auto review代码审查报告总体评价这段代码对 具体分析1. 代码逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进建议
int Utils::executeCmd(const QString &strCmd, QString *stdout, QString *stderr, QString &error)
QProcess proc;
proc.setProgram(prog);
proc.setArguments(args);
if (!proc.start(QIODevice::ReadWrite)) {
error = proc.errorString();
return -1;
}
if (!proc.waitForFinished(30000)) { // 30秒超时
error = "Command execution timeout";
proc.kill();
return -1;
}
QString error;
int exitcode = Utils::executeCmd(cmd, nullptr, nullptr, error);
// 可以添加一个异步版本的executeCmd
static void executeCmdAsync(const QString &strCmd, std::function<void(int, QString, QString)> callback);
if (!isValidCommand(prog)) {
error = "Invalid command";
return -1;
}
{
QProcess proc;
// ... 使用proc
} // proc自动销毁总结：这次改进提高了代码的灵活性，但在错误处理、性能优化和安全性方面还有提升空间。建议按照上述建议进行优化，使代码更加健壮和安全。 |
lzwind
approved these changes
Sep 22, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, wangrong1069 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 |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
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
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.
In the previous version, the standard output of badblocks was mistakenly used for judgment. The new version uses the standard error output.
Log: Fix fail to verify bad sectors
Bug: https://pms.uniontech.com/bug-view-315835.html
Summary by Sourcery
Add a new executCmd overload to return separate stdout and stderr, update badblocks calls to use stderr for status checks, and correct bad sector verification logic
New Features:
Bug Fixes:
Enhancements:
Documentation: