chore: More logs for base and service#156
Merged
Merged
Conversation
Reviewer's GuideThe PR instruments the disk management service and core libraries with extensive qDebug/qWarning logging around method entry/exit, branch points, error conditions, and external command invocations to improve runtime observability and aid debugging. Sequence diagram for logging in DiskManagerService public API methodssequenceDiagram
participant User as actor User
participant DMS as DiskManagerService
participant PC as PartedCore
User->>DMS: Call public API method (e.g., mount, unmount, create)
DMS->>DMS: qDebug() log method entry
DMS->>DMS: checkAuthorization()
alt Not authorized
DMS->>DMS: qDebug() log authorization failure
DMS-->>User: Return error/false
else Authorized
DMS->>PC: Call corresponding PartedCore method
DMS->>DMS: qDebug() log before/after call
DMS-->>User: Return result
end
Sequence diagram for logging in Utils::findProgramInPath and command executionsequenceDiagram
participant Caller
participant Utils
Caller->>Utils: findProgramInPath(proName)
Utils->>Utils: qDebug() log entry and proName
alt proName is empty
Utils->>Utils: qDebug() log empty program name
Utils-->>Caller: Return empty string
else proName not empty
Utils->>Utils: Execute command
Utils-->>Caller: Return result
end
Sequence diagram for logging in PartedCore::getDevicesequenceDiagram
participant PC as PartedCore
participant Ped as PedDevice
PC->>PC: qDebug() log BEGIN with devicePath
PC->>Ped: ped_device_get(devicePath)
alt Device found
PC->>PC: qDebug() log device found
alt flush
PC->>PC: flushDevice
end
PC->>PC: qDebug() log END, device found
PC-->>Caller: Return true
else Device not found
PC->>PC: qDebug() log END, device not found
PC-->>Caller: Return false
end
Class diagram for logging additions in DiskManagerService and UtilsclassDiagram
class DiskManagerService {
+initConnection()
+Quit()
+Start()
+getDeviceinfo()
+getalldevice()
+onGetAllDeviceInfomation()
+setCurSelect(info)
+unmount()
+mount(mountpath)
+deCrypt(luks)
+cryptMount(luks)
+cryptUmount(luks)
+getallsupportfs()
+format(fstype, name)
+clear(wipe)
+resize(info)
+create(infovec)
+onGetDeviceHardInfo(devicepath)
+onGetDeviceHardStatus(devicepath)
+onGetDeviceHardStatusInfo(devicepath)
+onDeletePartition()
+onHidePartition()
+onShowPartition()
+onDetectionPartitionTableError(devicePath)
+onCreatePartitionTable(devicePath, ...)
+onCheckBadBlocksCount(devicePath, ...)
+onCheckBadBlocksTime(devicePath, ...)
+onFixBadBlocks(devicePath, ...)
+onCreateVG(vgName, devList, size)
+onCreateLV(vgName, lvList)
+onDeleteVG(vglist)
+onDeleteLV(lvlist)
+onResizeVG(vgName, devList, size)
+onResizeLV(lvAction)
+onMountLV(lvAction)
+onUmountLV(lvAction)
+onClearLV(lvAction)
+onDeletePVList(devList)
<<logging: qDebug() at entry, exit, and error branches>>
}
class Utils {
+findProgramInPath(proName)
+executCmd(strCmd, outPut, error)
+executWithInputOutputCmd(strCmdArg, inPut, outPut, error)
+regexpLabel(strText, strPatter)
+getPartitionTypeString(type)
+fileSystemTypeToString(type)
+getFileSystemSoftWare(fileSystemType)
+formatSize(sectors, sectorSize)
+sectorToUnit(sectors, sectorSize, sizeUnit)
+getMaxPartitionNameLength(tableType)
+LVMFormatSize(lvmSize)
+LVMSizeToUnit(lvmSize, sizeUnit)
+adjudicationPVDelete(lvmInfo, pvStrList, bigDataMove, realMovePvList)
+getCipherStr(cipher)
+getCipher(cipher)
<<logging: qDebug() at entry, exit, and branch points>>
}
Class diagram for logging additions in PartedCoreclassDiagram
class PartedCore {
+getDeviceinfo()
+getAllDeviceinfo()
+getAllLVMinfo()
+getAllLUKSinfo()
+getallsupportfs()
+getDeviceHardInfo(devicepath)
+getDeviceHardStatus(devicepath)
+getDeviceHardStatusInfo(devicepath)
+createPartitionTable(devicePath, ...)
+detectionPartitionTableError(devicePath)
+create(infovec)
+format(fstype, name)
+clear(wipe)
+resize(info)
+deletePartition()
+showPartition()
+hidePartition()
+setCurSelect(info)
+mountAndWriteFstab(mountpath, mountUid)
+unmount()
+cryptMount(luks)
+cryptUmount(luks)
+deCrypt(luks)
+updateUsb()
+updateUsbRemove()
+checkBadBlocks(devicePath, ...)
+fixBadBlocks(devicePath, ...)
+refreshFunc()
+createVG(vgName, devList, size)
+deleteVG(vglist)
+createLV(vgName, lvList)
+deleteLV(lvlist)
+resizeVG(vgName, devList, size)
+resizeLV(lvAct)
+mountLV(lvAction)
+umountLV(lvAction)
+clearLV(lvAction)
+deletePVList(devList)
+test()
+setDeviceFromDisk(device, devicePath)
+useableDevice(lpDevice)
+delTempMountFile()
+initConnection()
+findSupportedCore()
+setDeviceSerialNumber(device)
+gptIsExpanded(devicePath)
+autoMount()
+autoUmount()
+probeDeviceInfo(...)
+onRefreshDeviceInfo(type, arg1, arg2)
+syncDeviceInfo(...)
+secuClear(path, start, end, size, fstype, name, count)
+getDevice(devicePath, lpDevice, flush)
+getDisk(lpDevice, lpDisk, strict)
+getDeviceAndDisk(devicePath, lpDevice, lpDisk, strict, flush)
+destroyDeviceAndDisk(lpDevice, lpDisk)
+isBusy(fstype, path, lpPartition)
+flushDevice(lpDevice)
+commit(lpDisk)
+commitToOs(lpDisk)
+infoBelongToPartition(partition, info)
+getLpPartition(lpDisk, partition)
+setDeviceOnePartition(device, lpDevice, fstype)
+setPartitionLabelAndUuid(partition)
+setMountPoints(partition)
+setMountPointsHelper(partition, path)
<<logging: qDebug() at entry, exit, and branch points>>
}
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 @re2zero - I've reviewed your changes - here's some feedback:
- This change introduces a massive number of qDebug calls; consider using QLoggingCategory or a custom logging wrapper so you can control verbosity and disable debug logs in release builds.
- Many of these debug statements are very low-level and repetitive; try consolidating key lifecycle events instead of tracing every branch to reduce log noise.
- Remove commented-out or duplicate log lines, and consider grouping related logs behind a single entry to keep the code and logs clean and maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This change introduces a massive number of qDebug calls; consider using QLoggingCategory or a custom logging wrapper so you can control verbosity and disable debug logs in release builds.
- Many of these debug statements are very low-level and repetitive; try consolidating key lifecycle events instead of tracing every branch to reduce log noise.
- Remove commented-out or duplicate log lines, and consider grouping related logs behind a single entry to keep the code and logs clean and maintainable.
## Individual Comments
### Comment 1
<location> `basestruct/utils.cpp:27` </location>
<code_context>
QString Utils::findProgramInPath(const QString &proName)
{
qDebug() << "Utils::findProgramInPath----" << proName;
- if (proName.isEmpty())
+ if (proName.isEmpty()) {
+ qDebug() << "Program name is empty";
return QString();
+ }
</code_context>
<issue_to_address>
Debug output in utility functions may clutter logs.
Since these functions are called often, excessive debug output can flood logs. Please minimize or conditionally enable debug statements here.
Suggested implementation:
```cpp
#ifdef UTILS_DEBUG
qDebug() << "Utils::findProgramInPath----" << proName;
#endif
if (proName.isEmpty()) {
#ifdef UTILS_DEBUG
qDebug() << "Program name is empty";
#endif
return QString();
}
```
```cpp
for (int i = 1; i < cmdList.size(); i++) {
if (!cmdList[i].isEmpty()) {
#ifdef UTILS_DEBUG
qDebug() << "Appending argument:" << cmdList[i];
#endif
argList.append(cmdList[i]);
}
}
```
To enable debug output, define the `UTILS_DEBUG` macro (e.g., by adding `-DUTILS_DEBUG` to your compiler flags). Otherwise, debug output will be suppressed.
</issue_to_address>
### Comment 2
<location> `service/diskoperation/filesystems/ntfs.cpp:76` </location>
<code_context>
QString cmd = QString("ntfsinfo --mft --force %1").arg(partition.getPath());
if (!Utils::executCmd(cmd, output, error)) {
+ qDebug() << "ntfsinfo executed successfully.";
strmatch = "Cluster Size:";
</code_context>
<issue_to_address>
The logic for checking ntfsinfo command success is inverted.
The current check executes the success block when the command fails, which is counterintuitive and may cause confusion or bugs. Please invert the logic or clarify with a comment.
</issue_to_address>
### Comment 3
<location> `service/diskoperation/filesystems/ntfs.cpp:164` </location>
<code_context>
exitcode = Utils::executCmd(QString("mkntfs -Q -v -F %1 -L %2").arg(newPartition.getPath()).arg(newPartition.getFileSystemLabel()),output, error);
}
- return exitcode == 0 && error.compare("Unknown error") == 0;
+ bool success = (exitcode == 0 && error.compare("Unknown error") == 0);
+ qDebug() << "[NTFS]::create - Exit with status:" << success;
+ return success;
</code_context>
<issue_to_address>
The NTFS create function only returns true if error is exactly 'Unknown error'.
The current condition may incorrectly report failures when the command succeeds but the error string differs. It's better to return true based solely on `exitcode == 0`.
</issue_to_address>
### Comment 4
<location> `service/diskoperation/filesystems/ntfs.cpp:146` </location>
<code_context>
qDebug() << "[NTFS]::writeUuid - Enter";
QString output, error;
int exitcode = Utils::executCmd(QString("ntfslabel --new-serial ").arg(partition.getPath()), output, error);
- return exitcode == 0 || error.compare("Unknown error") == 0;
- qDebug() << "[NTFS]::writeUuid - Exit";
</code_context>
<issue_to_address>
The ntfslabel command string is missing a placeholder for the device path.
The command string should include a '%1' placeholder so that the device path is correctly inserted and the command operates on the intended device.
</issue_to_address>
### Comment 5
<location> `service/diskoperation/filesystems/ntfs.cpp:118` </location>
<code_context>
qDebug() << "[NTFS]::readLabel - Enter";
QString output, error;
if (!Utils::executCmd(QString("ntfslabel --force").arg(partition.getPath()), output, error)) {
+ qDebug() << "Successfully read label:" << output.trimmed();
partition.setFilesystemLabel(output.trimmed());
</code_context>
<issue_to_address>
The ntfslabel command for reading label is missing the device path.
`ntfslabel --force` is run without a device path, which may cause it to fail. Please include the device path in the command.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -65,33 +74,36 @@ void NTFS::setUsedSectors( Partition & partition ) | |||
| QString cmd = QString("ntfsinfo --mft --force %1").arg(partition.getPath()); | |||
|
|
|||
| if (!Utils::executCmd(cmd, output, error)) { | |||
There was a problem hiding this comment.
suggestion: The logic for checking ntfsinfo command success is inverted.
The current check executes the success block when the command fails, which is counterintuitive and may cause confusion or bugs. Please invert the logic or clarify with a comment.
| exitcode = Utils::executCmd(QString("mkntfs -Q -v -F %1 -L %2").arg(newPartition.getPath()).arg(newPartition.getFileSystemLabel()),output, error); | ||
| } | ||
| return exitcode == 0 && error.compare("Unknown error") == 0; | ||
| bool success = (exitcode == 0 && error.compare("Unknown error") == 0); |
There was a problem hiding this comment.
issue (bug_risk): The NTFS create function only returns true if error is exactly 'Unknown error'.
The current condition may incorrectly report failures when the command succeeds but the error string differs. It's better to return true based solely on exitcode == 0.
Add more logs for base and service. 77.11% Log: More logs for base and service.
lzwind
approved these changes
Jul 7, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, re2zero 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 |
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.
Add more logs for base and service. 77.11%
Log: More logs for base and service.
Summary by Sourcery
Add extensive debug logging throughout the disk manager codebase to improve observability of operations and ease troubleshooting.
Enhancements: