feat: implement config reload with smart file detection #117
Conversation
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
|
Warning
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @18202781743 - I've reviewed your changes - here's some feedback:
- detectChangedFiles() does a full recursive scan of all config directories on every reload—consider using QFileSystemWatcher or caching the file list between reloads to avoid performance hits in large directories.
- The isSystemTimeReset() logic reads /proc/uptime and compares boot times, which can misinterpret suspend/resume or fail on non‐Linux platforms—consider using a monotonic clock (e.g. CLOCK_BOOTTIME or QElapsedTimer) for more reliable time-drift detection.
- After removing the old isConfigurePath() logic, double-check that getAllConfigDirectories() still covers all your application-specific override paths to ensure no config files are inadvertently excluded from monitoring.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- detectChangedFiles() does a full recursive scan of all config directories on every reload—consider using QFileSystemWatcher or caching the file list between reloads to avoid performance hits in large directories.
- The isSystemTimeReset() logic reads /proc/uptime and compares boot times, which can misinterpret suspend/resume or fail on non‐Linux platforms—consider using a monotonic clock (e.g. CLOCK_BOOTTIME or QElapsedTimer) for more reliable time-drift detection.
- After removing the old isConfigurePath() logic, double-check that getAllConfigDirectories() still covers all your application-specific override paths to ensure no config files are inadvertently excluded from monitoring.
## Individual Comments
### Comment 1
<location> `dconfig-center/dde-dconfig-daemon/dconfigserver.cpp:556` </location>
<code_context>
+ QDateTime currentTime = QDateTime::currentDateTimeUtc();
+
+ // Check if system boot time changed (system reboot)
+ if (qAbs(currentBootTime.secsTo(m_systemBootTime)) > 60) {
+ qCInfo(cfLog()) << "System reboot detected, not a time reset";
+ return false; // System reboot, not time reset
</code_context>
<issue_to_address>
The 60-second threshold for system boot time difference may be too strict or too loose.
Since system clock drift or delays could affect this logic, consider making the threshold configurable or clearly documenting why 60 seconds was chosen.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Check if system boot time changed (system reboot)
if (qAbs(currentBootTime.secsTo(m_systemBootTime)) > 60) {
qCInfo(cfLog()) << "System reboot detected, not a time reset";
return false; // System reboot, not time reset
}
=======
// Threshold (in seconds) for detecting system reboot via boot time change.
// Default is 60 seconds to allow for minor clock drift or delays.
static const int systemBootTimeThresholdSecs = 60; // TODO: Make configurable if needed
// Check if system boot time changed (system reboot)
if (qAbs(currentBootTime.secsTo(m_systemBootTime)) > systemBootTimeThresholdSecs) {
qCInfo(cfLog()) << "System reboot detected, not a time reset";
return false; // System reboot, not time reset
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `dconfig-center/dde-dconfig-daemon/dconfigserver.cpp:725` </location>
<code_context>
+ }
+
+ // Check file extension
+ if (!filePath.endsWith(".json")) {
+ return false;
+ }
</code_context>
<issue_to_address>
File extension check is case-sensitive.
Currently, files with uppercase extensions like ".JSON" will not be recognized. Normalize the extension to ensure case-insensitive matching if needed.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// Check file extension
if (!filePath.endsWith(".json")) {
return false;
}
=======
// Check file extension (case-insensitive)
if (!filePath.toLower().endsWith(".json")) {
return false;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `dconfig-center/dde-dconfig-daemon/dconfigserver.cpp:507` </location>
<code_context>
+ * 2. Signature verification: If time changed, verify with file signature comparison
+ * This prevents false positives from time inaccuracies while maintaining efficiency.
+ */
+void DSGConfigServer::reload()
+{
+ qCInfo(cfLog()) << "Reload configuration files";
</code_context>
<issue_to_address>
Consider moving all reload, scanning, and file signature logic into a separate ConfigReloadManager helper class to keep DSGConfigServer focused on core responsibilities.
```markdown
Consider pulling all of the “reload / scanning / signature / boot-time” logic out of DSGConfigServer into its own helper (e.g. `ConfigReloadManager`). This keeps your core server class focused on D-Bus and resource management, and hides all the file-watching complexity behind a small, well-tested interface.
1. Create a new header `configreloadmanager.h`:
```cpp
// configreloadmanager.h
#pragma once
#include <QObject>
#include <QStringList>
#include <QDateTime>
class ConfigReloadManager : public QObject {
Q_OBJECT
public:
explicit ConfigReloadManager(const QString &localPrefix, QObject *parent = nullptr);
/// Initialize on startup (boots, initial signatures…)
void initialize();
/// Scans & returns a list of files that have changed since last reload
QStringList reload();
signals:
/// Emitted when reload() finds one or more changed files
void filesChanged(const QStringList &changedFiles);
private:
QString m_localPrefix;
QDateTime m_startupTime;
QDateTime m_lastReloadTime;
QMap<QString, FileSignature> m_fileSignatures;
QDateTime getSystemBootTime() const;
bool isSystemTimeReset() const;
QStringList getAllConfigDirectories() const;
QStringList findConfigFiles(const QStringList &dirs) const;
FileSignature getFileSignature(const QString &path) const;
bool isFileChanged(const FileSignature &, const FileSignature &, const QDateTime &) const;
};
```
2. Move *all* of your `reload()`, `detectChangedFiles()`, `isSystemTimeReset()`, `getAllConfigDirectories()`, etc. into `ConfigReloadManager::reload()` and its private helpers. The new `.cpp` is roughly your existing code, but now lives in one file.
3. In your `DSGConfigServer`:
```cpp
// .h
#include "configreloadmanager.h"
class DSGConfigServer : public QObject {
…
private:
ConfigReloadManager *m_reloadMgr;
};
// .cpp (in your initialize or constructor)
m_reloadMgr = new ConfigReloadManager(m_localPrefix, this);
m_reloadMgr->initialize();
connect(m_reloadMgr, &ConfigReloadManager::filesChanged,
this, &DSGConfigServer::onFilesChanged);
```
4. Delegate reload calls:
```cpp
void DSGConfigServer::onFilesChanged(const QStringList &files) {
for (auto &f : files)
update(f);
}
// Whenever you want to trigger a manual reload:
void DSGConfigServer::reload() {
auto changed = m_reloadMgr->reload();
if (!changed.isEmpty())
emit m_reloadMgr->filesChanged(changed);
}
```
Result:
- **DSGConfigServer** remains ~200 lines of D-Bus / resource logic.
- **ConfigReloadManager** encapsulates all scanning, timestamp, signature and boot-time logic.
- You can unit-test reload behavior in isolation and avoid mixing low-level FS code into your core server.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Reviewer's GuideThis PR adds a new D-Bus reload interface with smart file detection—using multi-level checks (ctime + signature), system time reset handling, and signature caching—enhances directory scanning and validation, replaces the old trigger handler script, and updates related documentation. Sequence diagram for the new D-Bus reload interface with smart config file detectionsequenceDiagram
actor User
participant DConfigClient as D-Bus Client
participant DConfigServer as DSGConfigServer
participant FileSystem
User->>DConfigClient: Triggers reload (e.g., via dbus-send)
DConfigClient->>DConfigServer: Call reload()
DConfigServer->>DConfigServer: Check for system time reset
DConfigServer->>DConfigServer: Scan config directories
DConfigServer->>FileSystem: Get file metadata (ctime, size)
DConfigServer->>DConfigServer: Compare with cached signatures
alt Changed files detected
DConfigServer->>DConfigServer: Call update(filePath) for each changed file
end
DConfigServer->>DConfigServer: Update last reload time and cache
DConfigServer-->>DConfigClient: Return (void)
DConfigClient-->>User: Reload complete
Class diagram for DSGConfigServer with reload and smart detectionclassDiagram
class DSGConfigServer {
+void initialize()
+void reload()
+QDateTime getSystemBootTime() const
+bool isSystemTimeReset() const
+QStringList getAllConfigDirectories() const
+QStringList findConfigFiles(const QStringList&) const
+FileSignature getFileSignature(const QString&) const
+QStringList detectChangedFiles() const
+void initializeFileSignatures()
+bool isFileChanged(const FileSignature&, const FileSignature&, const QDateTime&) const
+bool isValidConfigFile(const QString&) const
QDateTime m_startupTime
QDateTime m_systemBootTime
QDateTime m_lastReloadTime
QMap<QString, FileSignature> m_fileSignatures
}
class FileSignature {
qint64 size
QDateTime changeTime
QString filePath
bool operator==(const FileSignature&) const
}
DSGConfigServer "1" o-- "*" FileSignature : m_fileSignatures
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
mhduiy
reviewed
Jul 2, 2025
|
Warning
|
mhduiy
previously approved these changes
Jul 2, 2025
zccrs
requested changes
Jul 2, 2025
|
Warning
|
|
Warning
|
zccrs
requested changes
Jul 3, 2025
zccrs
requested changes
Jul 3, 2025
|
Warning
|
deepin pr auto review关键摘要:
是否建议立即修改: |
|
Warning
|
zccrs
previously approved these changes
Jul 3, 2025
1. Added new reload() method to detect and update changed config files 2. Implemented file signature tracking to detect actual changes 3. Replaced bash trigger handler with simpler reload handler 4. Added initialize() method to track initial file states 5. Updated D-Bus interface and documentation for new reload feature 6. Modified Debian triggers to use new reload mechanism The changes improve configuration file change detection by: - Using file metadata (size + change time) instead of timestamps - Scanning all relevant config directories (including overrides) - Only updating actually changed files - Removing complex bash script in favor of native C++ implementation feat: 实现配置文件重新加载机制 1. 新增reload()方法检测并更新变更的配置文件 2. 实现文件签名跟踪以检测实际变更 3. 用更简单的重载处理器替换bash触发器处理程序 4. 添加initialize()方法跟踪初始文件状态 5. 更新D-Bus接口和文档以支持新重载功能 6. 修改Debian触发器使用新重载机制 这些改进通过以下方式提升配置文件变更检测: - 使用文件元数据(大小+变更时间)而非时间戳 - 扫描所有相关配置目录(包括覆盖目录) - 仅更新实际变更的文件 - 移除复杂的bash脚本改用原生C++实现
|
Warning
|
mhduiy
approved these changes
Jul 3, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
detection
comparison)
Key technical details:
feat: 实现智能检测的配置重载功能
关键技术细节:
Summary by Sourcery
Introduce a new
reloadD-Bus interface to automatically detect and apply configuration file changes across all standard directories using multi-level detection (ctime and file signature), handle system time resets, and cache file signatures for efficiency.New Features:
reloadD-Bus method for automatic config reloadingEnhancements:
Build:
Documentation:
reloadinterface