Conversation
When a user creates and mounts a logical volume on a removable device using deepin-diskmanager, there's no graphical way to stop the logical volume except by deleting the logical volume group. This prevents the user from safely removing the removable device, and even if they do manage to remove it safely, subsequent mounts of the logical volume will fail because it wasn't stopped. Therefore, let's exclude removable devices when creating logical volume groups. Log: Exclude removable devices when creating logical volume groups Bug: https://pms.uniontech.com/bug-view-340061.html
As title. Log: Update version to 6.0.22
|
TAG Bot TAG: 6.0.22 |
Reviewer's GuideIntroduce a mechanism to detect and filter out removable devices when generating the available disk list by adding a helper function and integrating it into createAvailableDiskData. Class diagram for CreateVGWidget and isDeviceRemovable integrationclassDiagram
class CreateVGWidget {
+QList<DeviceInfo> createAvailableDiskData()
+void updateData()
}
class isDeviceRemovable {
+static bool isDeviceRemovable(QString devicePath)
}
CreateVGWidget --> isDeviceRemovable : uses
Flow diagram for filtering removable devices in createAvailableDiskDataflowchart TD
A["Start createAvailableDiskData()"] --> B["Iterate over disk list"]
B --> C["Check if device is removable (isDeviceRemovable)"]
C -->|Removable| D["Skip device"]
C -->|Not removable| E["Continue processing device"]
E --> F["Check if device is in VG or has partition table error"]
F --> G["Add to available disk list if valid"]
D --> B
G --> H["Return available disk list"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这段代码进行审查分析：
改进建议：
static bool isDeviceRemovable(const QString &devicePath)
{
QFileInfo deviceInfo(devicePath);
QString deviceName = deviceInfo.fileName();
QString sysfsRemovablePath = QString("/sys/block/%1/removable").arg(deviceName);
QFile removableFile(sysfsRemovablePath);
if (!removableFile.exists()) {
qWarning() << "Sysfs path does not exist:" << sysfsRemovablePath;
return false;
}
if (!removableFile.open(QIODevice::ReadOnly | QIODevice::Text)) {
qWarning() << "Could not open sysfs removable file:" << sysfsRemovablePath;
return false;
}
QByteArray content = removableFile.readAll().trimmed();
bool removable = (content == "1");
qDebug() << "isDeviceRemovable:" << devicePath << removable;
return removable;
}
static QHash<QString, bool> removableDeviceCache;
static bool isDeviceRemovable(const QString &devicePath)
{
if (removableDeviceCache.contains(devicePath)) {
return removableDeviceCache[devicePath];
}
// ... 原有的检查逻辑 ...
removableDeviceCache[devicePath] = removable;
return removable;
}
static bool isDeviceRemovable(const QString &devicePath)
{
if (devicePath.isEmpty()) {
qWarning() << "Empty device path provided";
return false;
}
QFileInfo deviceInfo(devicePath);
if (!deviceInfo.exists()) {
qWarning() << "Device path does not exist:" << devicePath;
return false;
}
// ... 其余逻辑 ...
}
static bool isDeviceRemovable(const QString &devicePath)
{
// 首先检查是否是块设备
QFileInfo deviceInfo(devicePath);
if (!deviceInfo.isSymLink()) {
// 检查是否是实际的块设备
QString canonicalPath = deviceInfo.canonicalFilePath();
if (!canonicalPath.startsWith("/dev/")) {
qWarning() << "Invalid device path:" << devicePath;
return false;
}
}
// ... 其余逻辑 ...
}这些改进将使代码更加健壮、安全和高效，同时符合现代C++的最佳实践。 |
![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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `application/widgets/createvgwidget.cpp:1153-1156` </location>
<code_context>
+ bool removable = false;
+ QByteArray content;
+
+ if (!removableFile.exists()) {
+ qWarning() << "Sysfs path does not exist:" << sysfsRemovablePath;
+ goto quit;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Consider avoiding 'goto' for control flow in favor of clearer alternatives.
Refactor the logic to use early returns or other control structures instead of 'goto' to enhance readability and maintainability.
```suggestion
if (!removableFile.exists()) {
qWarning() << "Sysfs path does not exist:" << sysfsRemovablePath;
return false;
}
```
</issue_to_address>
### Comment 2
<location> `application/widgets/createvgwidget.cpp:1143` </location>
<code_context>
qDebug() << "CreateVGWidget::updateData completed";
}
+static bool isDeviceRemovable(const QString &devicePath)
+{
+ QFileInfo deviceInfo(devicePath);
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the goto in isDeviceRemovable() with early returns and flattening the logic for better readability.
Consider flattening isDeviceRemovable() with early returns and removing the goto. For example:
```cpp
static bool isDeviceRemovable(const QString &devicePath)
{
const QString deviceName = QFileInfo(devicePath).fileName();
const QString sysfsPath = QString("/sys/block/%1/removable").arg(deviceName);
QFile f(sysfsPath);
if (!f.exists()) {
qWarning() << "Sysfs path does not exist:" << sysfsPath;
return false;
}
if (!f.open(QIODevice::ReadOnly | QIODevice::Text)) {
qWarning() << "Could not open sysfs removable file:" << sysfsPath;
return false;
}
const bool removable = (f.readAll().trimmed() == "1");
qDebug() << "isDeviceRemovable:" << devicePath << removable;
return removable;
}
```
If this helper is only used in createAvailableDiskData(), you can inline it:
```cpp
for (auto dev = deviceInfoMap.begin(); dev != deviceInfoMap.end(); ++dev) {
const QString path = dev.value().m_path;
if (path.isEmpty() || path.contains("/dev/mapper"))
continue;
// inline isDeviceRemovable
QString name = QFileInfo(path).fileName();
QFile rem(QString("/sys/block/%1/removable").arg(name));
if (rem.exists() && rem.open(QIODevice::ReadOnly)) {
if (rem.readAll().trimmed() == "1")
continue;
}
// …rest of your checks…
}
```
This removes the nested `if`s and the `goto`, making the flow 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.
lzwind
approved these changes
Nov 13, 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
|
/merge |
This was referenced Nov 13, 2025
Open
Open
Open
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 by Sourcery
Add detection of removable block devices and filter them out when gathering available disks in CreateVGWidget.
Enhancements: