feat(storage): add MMC storage vendor identification via sysfs#700
Conversation
Reviewer's GuideAdds MMC/eMMC vendor identification by reading manfid/oemid/name from sysfs with a new safe sysfs file reader, and maps IDs to vendor names via lookup tables in DeviceStorage. Sequence diagram for MMC vendor identification via sysfssequenceDiagram
participant DeviceStorage
participant Common
participant Sysfs
DeviceStorage->>DeviceStorage: setHwinfoInfo(mapInfo)
DeviceStorage->>DeviceStorage: check m_Driver contains mmcblk
DeviceStorage->>DeviceStorage: read SysFS Device Link
DeviceStorage->>Common: safeReadSysFsFile(sysfsDeviceLink, name, err)
Common->>Sysfs: read name
Sysfs-->>Common: name content
Common-->>DeviceStorage: name or err
DeviceStorage->>DeviceStorage: set m_Name if err empty
DeviceStorage->>Common: safeReadSysFsFile(sysfsDeviceLink, manfid, err)
Common->>Sysfs: read manfid
Sysfs-->>Common: manfid content
Common-->>DeviceStorage: manfid or err
DeviceStorage->>DeviceStorage: getManfName(manfIdRaw)
DeviceStorage->>DeviceStorage: set m_Vendor from MANFID_TABLE
DeviceStorage->>Common: safeReadSysFsFile(sysfsDeviceLink, oemid, err)
Common->>Sysfs: read oemid
Sysfs-->>Common: oemid content
Common-->>DeviceStorage: oemid or err
DeviceStorage->>DeviceStorage: [m_Vendor empty] getOemName(oemIdRaw)
DeviceStorage->>DeviceStorage: set m_Vendor from OEMID_TABLE
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 2 issues, and left some high level feedback:
- In
Common::safeReadSysFsFile, theisFile()/isSymLink()check and error message don’t align (you currently reject symlinks while saying "or symlink"); consider clarifying the intent (e.g., explicitly allow regular files and/or symlinks) and also handle the case wherecanonicalFilePath()returns an empty string for non‑existent paths before proceeding. DeviceStorage::oemIdHexToAsciiis declared public but never used; consider either removing it or making it private and adding a focused use site to avoid exposing dead API.- The global
MANFID_TABLEandOEMID_TABLEmaps are initialized at static scope; to avoid static initialization order issues and keep their scope tight, consider wrapping them in helper functions with function‑localstatic constmaps.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Common::safeReadSysFsFile`, the `isFile()/isSymLink()` check and error message don’t align (you currently reject symlinks while saying "or symlink"); consider clarifying the intent (e.g., explicitly allow regular files and/or symlinks) and also handle the case where `canonicalFilePath()` returns an empty string for non‑existent paths before proceeding.
- `DeviceStorage::oemIdHexToAscii` is declared public but never used; consider either removing it or making it private and adding a focused use site to avoid exposing dead API.
- The global `MANFID_TABLE` and `OEMID_TABLE` maps are initialized at static scope; to avoid static initialization order issues and keep their scope tight, consider wrapping them in helper functions with function‑local `static const` maps.
## Individual Comments
### Comment 1
<location path="deepin-devicemanager/src/DeviceManager/DeviceStorage.cpp" line_range="870-873" />
<code_context>
+}
+
+// 通过manfid获取厂商名称
+QString DeviceStorage::getManfName(const QString &rawManfId)
+{
+ QString cleanId = rawManfId.trimmed().toLower();
+ cleanId.remove("0x"); // 去除0x前缀
+ return MANFID_TABLE.value(cleanId, rawManfId);
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing "0x" this way will also strip any '0' and 'x' characters inside the ID
`cleanId.remove("0x")` removes all occurrences of that substring, not just a leading prefix, so IDs containing `0x` in the middle will be corrupted (e.g. `"0x10x1b"` → `"11b"`), breaking lookups. Restrict this to a leading prefix only, e.g.:
```cpp
QString cleanId = rawManfId.trimmed().toLower();
if (cleanId.startsWith("0x")) {
cleanId.remove(0, 2);
}
```
Apply the same fix to `getOemName`.
</issue_to_address>
### Comment 2
<location path="deepin-devicemanager/src/commonfunction.cpp" line_range="433-435" />
<code_context>
+ return "";
+ }
+
+ if (!fullPathInfo.isFile() || fullPathInfo.isSymLink())
+ {
+ errOut = QString("Not a regular file or symlink: %1").arg(fullPath);
+ return "";
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Logic and message around regular file vs symlink are inconsistent and may reject valid sysfs paths
The condition `!fullPathInfo.isFile() || fullPathInfo.isSymLink()` actually rejects symlinks, while the message claims regular files *or* symlinks are allowed, and sysfs commonly exposes attributes via symlinks.
To allow both, invert the logic:
```cpp
if (!fullPathInfo.isFile() && !fullPathInfo.isSymLink()) {
errOut = QString("Not a regular file or symlink: %1").arg(fullPath);
return "";
}
```
If symlinks are meant to be disallowed, adjust the error message to match that behavior instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| QString DeviceStorage::getManfName(const QString &rawManfId) | ||
| { | ||
| QString cleanId = rawManfId.trimmed().toLower(); | ||
| cleanId.remove("0x"); // 去除0x前缀 |
There was a problem hiding this comment.
issue (bug_risk): Removing "0x" this way will also strip any '0' and 'x' characters inside the ID
cleanId.remove("0x") removes all occurrences of that substring, not just a leading prefix, so IDs containing 0x in the middle will be corrupted (e.g. "0x10x1b" → "11b"), breaking lookups. Restrict this to a leading prefix only, e.g.:
QString cleanId = rawManfId.trimmed().toLower();
if (cleanId.startsWith("0x")) {
cleanId.remove(0, 2);
}Apply the same fix to getOemName.
| if (!fullPathInfo.isFile() || fullPathInfo.isSymLink()) | ||
| { | ||
| errOut = QString("Not a regular file or symlink: %1").arg(fullPath); |
There was a problem hiding this comment.
issue (bug_risk): Logic and message around regular file vs symlink are inconsistent and may reject valid sysfs paths
The condition !fullPathInfo.isFile() || fullPathInfo.isSymLink() actually rejects symlinks, while the message claims regular files or symlinks are allowed, and sysfs commonly exposes attributes via symlinks.
To allow both, invert the logic:
if (!fullPathInfo.isFile() && !fullPathInfo.isSymLink()) {
errOut = QString("Not a regular file or symlink: %1").arg(fullPath);
return "";
}If symlinks are meant to be disallowed, adjust the error message to match that behavior instead.
add-uos
force-pushed
the
develop/eagle
branch
2 times, most recently
from
d544fca to
67a0ca2
Compare
Read manfid/oemid/name from sysfs for MMC storage devices and resolve vendor names using MANFID/OEMID lookup tables. Add safeReadSysFsFile() utility with path traversal protection. Log: 添加MMC存储设备厂商识别功能，通过sysfs读取manfid/oemid识别厂商 PMS: BUG-368319 Influence: MMC/eMMC存储设备的厂商信息不再为空，可正确显示对应厂商名称
deepin pr auto review★ 总体评分：85分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 --- a/deepin-devicemanager/src/commonfunction.cpp
+++ b/deepin-devicemanager/src/commonfunction.cpp
@@ -426,8 +426,14 @@ QString Common::safeReadSysFsFile(const QString &baseDir, const QString &fileNa
return "";
}
- if (!fullPathInfo.isFile() || fullPathInfo.isSymLink())
+ if (!fullPathInfo.isFile())
{
- errOut = QString("Not a regular file or symlink: %1").arg(fullPath);
+ errOut = QString("Target is not a regular file: %1").arg(fullPath);
+ return "";
+ }
+ if (fullPathInfo.isSymLink())
+ {
+ errOut = QString("Target is a symbolic link (not allowed): %1").arg(fullPath);
return "";
}
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: add-uos, 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 |
|
/merge |
3 participants
Read manfid/oemid/name from sysfs for MMC storage devices and resolve vendor names using MANFID/OEMID lookup tables. Add safeReadSysFsFile() utility with path traversal protection.
Log: 添加MMC存储设备厂商识别功能，通过sysfs读取manfid/oemid识别厂商
PMS: BUG-368319
Influence: MMC/eMMC存储设备的厂商信息不再为空，可正确显示对应厂商名称
Summary by Sourcery
Add vendor identification for MMC/eMMC storage devices using sysfs data and introduce a safe sysfs file reading utility.
New Features:
Bug Fixes:
Enhancements: