fix(disk): propagate SetPartionLabel errors to installer layer#123
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR makes disk partition labeling failures detectable by callers, hardens volume ID parsing, and improves error handling around filesystem checks during bootloader installation for both x86 and MIPS installers. Sequence diagram for updated bootloader installation error handlingsequenceDiagram
participant QtX86Installer
participant DiskUtil
participant fsck
participant isoinfo
QtX86Installer->>DiskUtil: SetPartionLabel(partitionName, image)
DiskUtil->>fsck: run_fsck(device)
alt [fsck failure]
fsck-->>DiskUtil: failure
Note right of DiskUtil: warn and continue
else [fsck success]
fsck-->>DiskUtil: success
end
DiskUtil->>isoinfo: get_volume_id(image)
alt [volume id parsed]
isoinfo-->>DiskUtil: volume_id
DiskUtil-->>QtX86Installer: true
QtX86Installer->>QtX86Installer: installBootload completes
else [volume id error]
isoinfo-->>DiskUtil: error
DiskUtil-->>QtX86Installer: false
QtX86Installer->>QtX86Installer: qCritical Failed to set partition label
QtX86Installer-->>QtX86Installer: installBootload returns false
end
Note over QtX86Installer,DiskUtil: QtMipsInstaller follows the same interaction with SetPartionLabel
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, and left some high level feedback:
- Now that
SetPartionLabelpropagates failure via a bool, consider returning or logging more contextual information (e.g., device name, image path, underlying error string) so callers and logs can distinguish different failure causes. - The
SetPartionLabelAPI still uses the misspelled name; if feasible, introduce a correctly spelled wrapper (e.g.,SetPartitionLabel) and deprecate the old one to avoid further propagation of the typo in new code. - In the installer callers, consider including the partition name and image in the
qCriticalmessage when label setting fails to make troubleshooting easier in multi-disk environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `SetPartionLabel` propagates failure via a bool, consider returning or logging more contextual information (e.g., device name, image path, underlying error string) so callers and logs can distinguish different failure causes.
- The `SetPartionLabel` API still uses the misspelled name; if feasible, introduce a correctly spelled wrapper (e.g., `SetPartitionLabel`) and deprecate the old one to avoid further propagation of the typo in new code.
- In the installer callers, consider including the partition name and image in the `qCritical` message when label setting fails to make troubleshooting easier in multi-disk environments.
## Individual Comments
### Comment 1
<location path="src/libdbm/installer/qtX86Installer.cpp" line_range="50-52" />
<code_context>
qDebug() << "Setting partition label for image:" << m_strImage;
- XSys::DiskUtil::SetPartionLabel(m_strPartionName, m_strImage);
+ if (!XSys::DiskUtil::SetPartionLabel(m_strPartionName, m_strImage)) {
+ qCritical() << "Failed to set partition label";
+ return false;
+ }
</code_context>
<issue_to_address>
**suggestion:** Include more context in the critical log to aid diagnosis.
Consider including `m_strPartionName` and/or `m_strImage` in this critical log so operators can identify which partition/image failed, especially when multiple installs run in parallel or logs are aggregated.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Make SetPartionLabel return bool and check its result in both QtX86Installer and QtMipsInstaller, so partition label failures no longer silently pass. Correctly calling the fatlabel command. 修改SetPartionLabel返回bool，在Installer层检查返回值， 分区标签设置失败时及时终止安装流程。正确调用fatlabel命令。 Log: 修复分区标签设置失败未上报的问题 PMS: BUG-361883 Influence: 分区标签设置失败时安装流程会正确报错终止，避免产生 标签异常的启动盘。
deepin pr auto review你好！我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次代码变更主要涉及：版权年份更新、错误处理增强（ 总体来说，这次修改大幅度提升了代码的健壮性、安全性和可读性。以下是针对各个维度的详细审查意见和进一步优化建议： 1. 语法与逻辑👍 亮点：
🛠️ 改进建议：
2. 代码质量👍 亮点：
🛠️ 改进建议：
3. 代码性能👍 亮点：
🛠️ 改进建议：
4. 代码安全 🚨👍 亮点（重大安全修复）：
🛠️ 改进建议：
总结与重构后代码参考本次代码变更质量很高，尤其是安全漏洞的修复和异常处理的增强。建议按照上述意见微调后合入。 以下是整合了改进建议的 bool SetPartionLabel(const QString& strPartion, const QString& strImage)
{
if (!XSys::SynExec(XSys::FS::SearchBin("fsck"), QStringList{"-y", strPartion}).isSuccess()) {
qWarning() << "fsck failed on" << strPartion << ", continuing anyway";
}
XSys::Result isoResult = XSys::SynExec("isoinfo", QStringList{"-i", strImage, "-d"});
if (!isoResult.isSuccess()) {
qWarning() << "call isoinfo failed:" << isoResult.result();
return false;
}
// Volume id 格式: "Volume id: DEEPIN" 或 "Volume id: UOS"
QString volumeId;
for (const QString &line : isoResult.result().split("\n")) {
if (line.startsWith("Volume id:")) {
// 使用 mid 代替 split(":").last()，防止卷标中包含冒号导致被截断
volumeId = line.mid(line.indexOf(':') + 1).trimmed();
break;
}
}
if (volumeId.isEmpty()) {
qWarning() << "No volume id found in isoinfo output";
return false;
}
// FAT32 卷标最长 11 字符
QString label = volumeId.left(11);
if (volumeId.length() > 11) {
qWarning() << "Volume id truncated:" << volumeId << "->" << label;
}
// 使用 QStringList 防止命令注入，无需手动拼接空格和引号
return XSys::SynExec(XSys::FS::SearchBin("fatlabel"), QStringList{strPartion, label}).isSuccess();
} |
lzwind
approved these changes
May 21, 2026
|
[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.
Summary
SetPartionLabelreturn type fromvoidtoboolto propagate errorsfsckfailure gracefully (warn but continue)QtX86Installer,QtMipsInstaller) now check return value and abort on failureTest plan
Summary by Sourcery
Bug Fixes: