refactor(dde-dconfig-daemon): P0/P1/P2 全量重构——连接管理、同步策略、热重载、服务框架、安全、路径管理、CLI、测试#145
Closed
18202781743 wants to merge 7 commits into
Closed
refactor(dde-dconfig-daemon): P0/P1/P2 全量重构——连接管理、同步策略、热重载、服务框架、安全、路径管理、CLI、测试#14518202781743 wants to merge 7 commits into
18202781743 wants to merge 7 commits into
Conversation
Changed all static function declarations to inline in helper.hpp to resolve multiple definition warnings during linking. This ensures each translation unit gets its own copy of the functions while maintaining the same behavior. Updated debug logging format specifiers from %d to %lld in multiple files to properly handle qint64 count values, preventing format string warnings. Changed range-based for loops to use const references instead of copies for QString objects to improve performance and avoid implicit copying warnings. These changes eliminate compilation warnings without affecting functionality, ensuring cleaner build output and better code quality. Influence: 1. Verify that all helper functions still work correctly after changing from static to inline 2. Test debug logging functionality to ensure new format specifiers work with qint64 values 3. Validate that range-based for loops with const references behave identically to previous value copies 4. Ensure no linking errors occur due to multiple definitions 5. Confirm that all existing functionality remains intact after these changes fix: 将静态函数改为内联函数以解决编译警告 将 helper.hpp 中的所有静态函数声明改为内联函数，以解决链接时的多重定义警 告。这确保每个翻译单元获得自己的函数副本，同时保持相同的行为。 在多个文件中将调试日志格式说明符从 %d 更改为 %lld，以正确处理 qint64 计 数类型，防止格式字符串警告。 将基于范围的 for 循环改为使用常量引用而不是 QString 对象的副本，以提高性 能并避免隐式复制警告。 这些更改消除了编译警告而不影响功能，确保更清晰的构建输出和更好的代码 质量。 Influence: 1. 验证将静态函数改为内联后，所有辅助函数是否仍能正常工作 2. 测试调试日志功能，确保新的格式说明符适用于 qint64 值 3. 验证使用常量引用的基于范围的 for 循环是否与先前的值复制行为相同 4. 确保不会因多重定义而发生链接错误 5. 确认这些更改后所有现有功能保持完整 Change-Id: I1477fcfd4ee2af683910a4307a4606226ea4e238
T04-1: 新增 dconfig_types.h，定义类型安全 ConnKey 结构体（toString/fromString/qHash） T04-2: RefManager 延迟释放改为单一全局定时器 + QHash<ConnKey,qint64> 到期队列 T04-3: m_resources/m_files/m_caches/m_conns 全部由 QMap 改为 QHash T04-4: DSGConfigServer::exit() 添加连接泄漏检测 warning Change-Id: If57d68a44f6ab48484e1b05e57414934bf3e9fce
T05-1: 新增 configsyncpolicy.h，抽象 ConfigSyncPolicy 接口（schedule/flush/clear） T05-2: ConfigSyncRequestCache 继承 ConfigSyncPolicy，实现 schedule/flush T05-3: DSGConfigResource::saveWithRetry() 最多3次重试，指数退避100ms T05-4: DSGConfigServer::exit() 开头调用 flush() 确保退出前写盘 Change-Id: Icffe0acc35dfdaaff3da1e073aaf0f8741e7f298
T06-1: 新增 inotifywatcher.h/.cpp，封装 Linux inotify 监听文件变化
T06-2: DSGConfigServer::initialize() 改为 InotifyWatcher 监听三个配置目录
移除 allConfigureFileSignatures() 轮询方案和 m_fileSignatures 成员
T06-3: 新增 m_reloadThrottle（200ms 节流）+ m_pendingReloadPaths 队列
T06-4: update() 成功后 emit configUpdated(appid, resource) 信号
reload() 改为手动触发节流队列立即刷出
Change-Id: Ie16aeeba93e1c03843754ed5af06a2916aab8e33
T01-1: 新增 ServiceLifecycle 状态机（Initializing→Running→Stopping→Stopped） T01-2: main.cpp 使用 socketpair 桥接 POSIX 信号到 Qt 事件，SIGTERM 有序关闭 T01-3: 启动时设置 QT_MESSAGE_PATTERN（时间戳+pid+category），支持运行时 setLogRules T07-1: removeUserData() 添加 AUDIT 级别日志（uid/caller/pid） T07-2: acquireManagerV2() 添加 uid 校验，非 root 只能访问自己的数据 T03-1: 新增 ConfigPathResolver 单例，支持优先级路径管理（metaPaths/overridePaths） T03-2: initialize() 使用 ConfigPathResolver 统一注册配置搜索路径 Change-Id: I60a1bb87ea39f91627ac95407765727e9910b2c2
T09-1: dde-dconfig --output json|text，list/get 支持 JSON 输出
T09-2: watchCommand 支持 --output json，输出 {key,appid,resource} 对象
T09-3: 新增 export/import 子命令，快照导出/导入全量配置值到 JSON 文件
T08-1: DSGConfigConn::value() 检测 [deprecated] 描述标记，发出 qCWarning
T08-2: DSGConfigConn::setValue() 解析 [schema:{...}] 描述，校验 type/min/max
校验失败返回 org.desktopspec.ConfigManager.InvalidValue D-Bus error
T11-1: 新增 ut_dconfigresource.cpp，覆盖 reparse/ServiceLifecycle/ConfigPathResolver
T11-3: 新增 .github/workflows/test.yml，CI 自动构建+测试+覆盖率报告
Change-Id: I7a870ae12078d0eb3ed8423f7e1169cf8514fb8a
T06 已用 InotifyWatcher 替代文件签名轮询，T03 cherry-pick 时 意外带入了 m_fileSignatures 旧代码，导致编译错误，现清理。 Change-Id: I2e039ddfb20669e4363288845ba548c4ea459e80
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743 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 |
|
Warning
详情 {
"export": {
"dconfig-center/dde-dconfig/main.cpp": {
"b": [
" int exportCommand(); // T09-3: export snapshot",
" QString snapshotFile; // T09-3: file path for export/import",
"// T09-3: export snapshot",
"int CommandManager::exportCommand()",
" outpuSTDError(\"export requires -a <appid> -r <resource> -o <file>\");",
" outpuSTDError(\"export requires --file/-o <snapshot.json>\");",
" // T09-3: --file for export/import",
" QCoreApplication::translate(\"main\", \"snapshot file path for export/import.\"), \"file\", QString());",
" QCommandLineOption exportOption(\"export\", QCoreApplication::translate(\"main\", \"export all config values to a JSON snapshot file.\"));",
" exportOption.setFlags(exportOption.flags() ^ QCommandLineOption::HiddenFromHelp);",
" parser.addOption(exportOption);",
" } else if (parser.isSet(exportOption) || exportOption.names().contains(subcommand)) {",
" return manager.exportCommand();"
]
}
},
"unset": {
"dconfig-center/tests/ut_dconfigresource.cpp": {
"b": [
" qunsetenv(\"DSG_CONFIG_CONNECTION_DISABLE_DBUS\");",
" qunsetenv(\"STATE_DIRECTORY\");"
]
}
}
} |
Reviewer's GuideSystematic refactor of dde-dconfig-daemon: introduces typed connection keys and a global ref manager timer, pluggable config sync policy with flush-on-exit and retry, inotify-based hot reload plus centralized path resolution, modernized daemon lifecycle with signal bridging and unified logging, strengthened security checks and audit logs, CLI JSON/export/import enhancements, schema/deprecated metadata handling, and new unit tests plus CI coverage workflow. Sequence diagram for POSIX signal handling and ordered shutdownsequenceDiagram
participant OS
participant Kernel
participant Daemon as dde_dconfig_daemon
participant SigHandler as posixSignalHandler
participant Socket as signal_socketpair
participant Notifier as QSocketNotifier
participant Lifecycle as ServiceLifecycle
participant Server as DSGConfigServer
participant ConfigSync
participant RefManager
OS->>Kernel: send SIGTERM/SIGINT
Kernel-->>SigHandler: deliver signal
SigHandler->>Socket: write(sig_byte)
Socket-->>Notifier: fd becomes readable
Notifier->>Daemon: activated(read)
Daemon->>Socket: read(sig_byte)
Daemon->>Lifecycle: transitionTo(Stopping)
Daemon->>Server: exit()
Server->>Server: log connection leaks
alt has m_syncRequestCache
Server->>ConfigSync: flush()
ConfigSync-->>Server: all pending writes flushed
end
Server->>RefManager: destroy()
Server->>Server: release resources
Daemon->>Lifecycle: transitionTo(Stopped)
Daemon->>Daemon: QCoreApplication::quit()
Sequence diagram for inotify-based hot reload and manual reloadsequenceDiagram
participant FS as Filesystem
participant Inotify as InotifyWatcher
participant Server as DSGConfigServer
participant Throttle as reloadThrottle(QTimer)
FS->>Inotify: file change in config dir
Inotify->>Server: fileChanged(path)
alt path endsWith(".json")
Server->>Server: enqueue path into m_pendingReloadPaths
Server->>Throttle: start(200ms)
end
Throttle-->>Server: timeout()
loop while m_pendingReloadPaths not empty
Server->>Server: dequeue path
Server->>Server: update(path)
alt update succeeds
Server-->>Clients: configUpdated(appid, resource)
else update fails
Server-->>DBusCaller: sendErrorReply()
end
end
Note over Client,Server: Manual reload() call
Client->>Server: reload()
Server->>Throttle: stop()
loop while m_pendingReloadPaths not empty
Server->>Server: dequeue path
Server->>Server: update(path)
end
Updated class diagram for core daemon componentsclassDiagram
class ConnKey {
+QString appid
+QString resource
+QString subpath
+uint uid
+QString toString()
+static ConnKey fromString(s: QString)
+bool isValid()
+bool operator==(o: ConnKey)
+bool operator!=(o: ConnKey)
}
class ServiceLifecycle {
-ServiceState m_state
+ServiceLifecycle(parent: QObject*)
+ServiceState state()
+bool isRunning()
+bool isStopping()
+bool transitionTo(next: ServiceState)
+static const char* stateName(s: ServiceState)
+signal stateChanged(from: ServiceState, to: ServiceState)
}
class ConfigSyncPolicy {
<<abstract>>
+ConfigSyncPolicy(parent: QObject*)
+void schedule(key: ConfigCacheKey)*
+void flush()*
+void clear()*
+signal syncConfigRequest(request: ConfigSyncBatchRequest)
}
class ConfigSyncRequestCache {
-QBasicTimer* m_syncTimer
-int m_delaySyncTime
-int m_batchCount
-QList~ConfigCacheKey~ m_configCacheKeys
+ConfigSyncRequestCache(parent: QObject*)
+~ConfigSyncRequestCache()
+void schedule(key: ConfigCacheKey)
+void flush()
+void clear()
+void pushRequest(key: ConfigCacheKey)
-void customRequest()
}
class RefManager {
-QHash~ConnServiceName, ServiceRef*~ services
-QHash~ConnKey, ResourceRef*~ resources
-int m_delayReleaseTime
-QTimer* m_globalReleaseTimer
-QHash~ConnKey, qint64~ m_pendingRelease
+RefManager(parent: QObject*)
+~RefManager()
+void destroy()
+void setDelayReleaseTime(ms: int)
+void delayDeleteResource(deleteResources: QList~ResourceRef*~)
-void doDeleteResource(deleteResources: QList~ResourceRef*~)
}
class InotifyWatcher {
-int m_fd
-QSocketNotifier* m_notifier
-QHash~int, QString~ m_wdToPath
-QHash~QString, int~ m_pathToWd
+InotifyWatcher(parent: QObject*)
+~InotifyWatcher()
+void addPath(path: QString)
+void removePath(path: QString)
+signal fileChanged(path: QString)
+signal directoryChanged(path: QString)
-void addWatchRecursive(path: QString)
-void onInotifyEvent()
}
class ConfigPathResolver {
-QString m_localPrefix
-QList~QPair~int, QString~~ m_paths
+static ConfigPathResolver& instance()
+void setLocalPrefix(prefix: QString)
+QString localPrefix() const
+void addSearchPath(path: QString, priority: int)
+void clearSearchPaths()
+QStringList searchPaths() const
+QStringList metaPaths(appid: QString, resource: QString) const
+QStringList overridePaths(appid: QString, resource: QString) const
+QStringList allWatchedDirs() const
}
class DSGConfigResource {
-QString m_key
-QString m_appid
-QString m_subpath
-QString m_localPrefix
-QHash~ResourceKey, DConfigFile*~ m_files
-QHash~ConnKey, DConfigCache*~ m_caches
-QHash~ConnKey, DSGConfigConn*~ m_conns
-ConfigSyncRequestCache* m_syncRequestCache
+void save()
+void save(appid: QString)
+bool saveWithRetry(cache: DConfigCache*, maxRetry: int)
+void removeConn(connKey: ConnKey)
+bool isEmptyConn() const
}
class DSGConfigConn {
+void setValue(key: QString, value: QDBusVariant)
+QDBusVariant value(key: QString)
-bool hasPermissionByUid(key: QString) const
-DConfigMeta* meta() const
}
class DSGConfigServer {
-QHash~GenericResourceKey, DSGConfigResource*~ m_resources
-RefManager* m_refManager
-ConfigSyncRequestCache* m_syncRequestCache
-InotifyWatcher* m_inotifyWatcher
-QTimer* m_reloadThrottle
-QQueue~QString~ m_pendingReloadPaths
-QString m_localPrefix
+DSGConfigServer(QObject* parent)
+~DSGConfigServer()
+bool registerService()
+void initialize()
+void exit()
+void reload()
+void update(path: QString)
+void removeUserData(uid: uint)
+QDBusObjectPath acquireManagerV2(uid: uint, appid: QString, name: QString, subpath: QString)
+signal configUpdated(appid: QString, resource: QString)
}
ConnKey <.. DSGConfigConn : uses
ConnKey <.. DSGConfigResource : key_for
ConnKey <.. RefManager : key_for
ConfigSyncPolicy <|-- ConfigSyncRequestCache
ConfigSyncRequestCache <.. DSGConfigServer : used_for_sync
ConfigSyncRequestCache <.. DSGConfigResource : used_for_sync
RefManager o--> DSGConfigResource : manages
DSGConfigServer o--> RefManager : owns
DSGConfigServer o--> DSGConfigResource : owns
DSGConfigServer o--> InotifyWatcher : owns
DSGConfigServer ..> ConfigPathResolver : uses_singleton
DSGConfigResource o--> DSGConfigConn : owns
ServiceLifecycle ..> DSGConfigServer : tracks_state_of
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review代码审查报告整体评价本次代码变更引入了多项重要改进，包括配置路径管理、文件监听机制、生命周期状态机、单元测试框架等。整体代码质量较高，但在某些方面仍有优化空间。 详细审查意见1. GitHub Actions 工作流 (test.yml)语法与逻辑
改进建议
2. helper.hpp 中的函数改进语法与逻辑
改进建议
3. ConfigPathResolver 实现语法与逻辑
改进建议
4. ConfigSyncPolicy 抽象接口语法与逻辑
改进建议
5. ConnKey 类型安全实现语法与逻辑
改进建议
6. DSGConfigConn 中的 JSON Schema 校验语法与逻辑
改进建议
7. RefManager 中的延迟释放机制语法与逻辑
改进建议
8. DSGConfigResource 中的保存重试机制语法与逻辑
改进建议
9. DSGConfigServer 中的 inotify 监听机制语法与逻辑
改进建议
10. InotifyWatcher 实现语法与逻辑
改进建议
11. ServiceLifecycle 状态机语法与逻辑
改进建议
12. main.cpp 中的信号处理语法与逻辑
改进建议
13. 单元测试 (ut_dconfigresource.cpp)语法与逻辑
改进建议
总结本次代码变更引入了多项重要改进，整体代码质量较高。主要改进点包括：
建议在以下几个方面进一步优化：
总体而言，本次代码变更是积极的，为项目带来了多项重要改进，建议在上述建议的基础上进一步完善代码。 |
![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 7 issues, and left some high level feedback:
- In ConfigPathResolver::overridePaths the condition
if (!m_localPrefix.isEmpty() || true)is always true, which likely defeats the intent of guarding the extra/etc/dsg/configs/overridespath and may generate incorrect/duplicate paths whenm_localPrefixis empty; consider removing the|| trueor tightening this condition. - The daemon main.cpp now calls dsgConfig.exit() both in the signal bridge lambda and again in the aboutToQuit() handler, which can result in double cleanup (and double flush/leak logs); it would be safer to centralize the shutdown logic so DSGConfigServer::exit() is only executed once.
- The new CLI export/import snapshot format currently serializes all values with
toString(), which loses type information for booleans, numbers and structured QVariant values; consider using QJsonValue::fromVariant / QJsonDocument::fromVariant so the snapshot preserves original types and can be round-tripped more faithfully.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In ConfigPathResolver::overridePaths the condition `if (!m_localPrefix.isEmpty() || true)` is always true, which likely defeats the intent of guarding the extra `/etc/dsg/configs/overrides` path and may generate incorrect/duplicate paths when `m_localPrefix` is empty; consider removing the `|| true` or tightening this condition.
- The daemon main.cpp now calls dsgConfig.exit() both in the signal bridge lambda and again in the aboutToQuit() handler, which can result in double cleanup (and double flush/leak logs); it would be safer to centralize the shutdown logic so DSGConfigServer::exit() is only executed once.
- The new CLI export/import snapshot format currently serializes all values with `toString()`, which loses type information for booleans, numbers and structured QVariant values; consider using QJsonValue::fromVariant / QJsonDocument::fromVariant so the snapshot preserves original types and can be round-tripped more faithfully.
## Individual Comments
### Comment 1
<location path="dconfig-center/dde-dconfig-daemon/dconfigserver.h" line_range="51-52" />
<code_context>
void tryExit();
+ /// T06-4: 配置文件更新时通知调用方（appid/resource 来自路径解析）
+ void configUpdated(const QString &appid, const QString &resource);
+
public Q_SLOTS:
</code_context>
<issue_to_address>
**issue (bug_risk):** configUpdated should be declared as a Qt signal if it is emitted
In dconfigserver.cpp, `configUpdated` is emitted (`emit configUpdated(...)`) but is declared here as a regular method instead of a signal. This will cause a compilation error or prevent it from working as a Qt signal. Please move this declaration into a `Q_SIGNALS:` section so `emit` is valid and it can be connected from other objects.
</issue_to_address>
### Comment 2
<location path="dconfig-center/dde-dconfig-daemon/inotifywatcher.cpp" line_range="10-12" />
<code_context>
+#include <QDir>
+#include <QDebug>
+
+#ifdef Q_OS_LINUX
+#include <sys/inotify.h>
+#include <unistd.h>
+#endif
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Missing errno/strerror includes for Linux branch
`errno` and `strerror(errno)` are used in the Linux code paths, but the file doesn’t include `<errno.h>` or `<string.h>/<cstring>`. Please add these headers explicitly rather than relying on transitive includes, which are undefined and may fail on some toolchains.
</issue_to_address>
### Comment 3
<location path="dconfig-center/dde-dconfig-daemon/configpathresolver.cpp" line_range="28-33" />
<code_context>
+
+void ConfigPathResolver::addSearchPath(const QString &path, int priority)
+{
+ const QString abs = m_localPrefix + path;
+ // 避免重复
+ for (const auto &p : m_paths) {
+ if (p.second == abs) return;
+ }
+ m_paths.append({priority, abs});
+ // 按 priority 降序排列
+ std::sort(m_paths.begin(), m_paths.end(), [](const auto &a, const auto &b) {
</code_context>
<issue_to_address>
**issue (bug_risk):** addSearchPath unconditionally prefixes with m_localPrefix, which can double-prefix or break absolute paths
Here `path` is always prefixed with `m_localPrefix`, but call sites already pass absolute paths (e.g. `"/usr/share/dsg/configs"`) and `linglongPath` is validated via `QDir(m_localPrefix + linglongPath).exists()` before being passed in. As a result, you can end up storing `m_localPrefix + m_localPrefix + linglongPath`, and absolute paths in general get incorrectly prefixed. Consider checking whether `path` is relative and only then prepending `m_localPrefix` (e.g. using `QDir::isRelativePath(path)`).
</issue_to_address>
### Comment 4
<location path="dconfig-center/dde-dconfig-daemon/configpathresolver.cpp" line_range="72-73" />
<code_context>
+ result << QString("%1/overrides/%2.json").arg(p.second, resource);
+ }
+ // 系统管理员覆盖目录
+ if (!m_localPrefix.isEmpty() || true) {
+ result << QString("%1/etc/dsg/configs/overrides/%2/%3.json")
+ .arg(m_localPrefix, appid, resource);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Condition `if (!m_localPrefix.isEmpty() || true)` is always true and likely not intended
Because of the `|| true`, this block always executes and the `/etc/dsg/configs/overrides/...` path is always added, making the `m_localPrefix.isEmpty()` check pointless and suggesting a leftover from debugging. If this path should only be added when a local prefix exists, drop `|| true`; if it should always be added, remove the `if` entirely for clarity.
</issue_to_address>
### Comment 5
<location path="dconfig-center/dde-dconfig-daemon/dconfigresource.cpp" line_range="454-459" />
<code_context>
void DSGConfigResource::save()
{
- qDebug(cfLog, "Save resource's cache for [%s], and cache count:%d", qPrintable(m_key), m_caches.count());
+ qDebug(cfLog, "Save resource's cache for [%s], and cache count:%lld", qPrintable(m_key), m_caches.count());
for (auto item : m_files)
item->save(m_localPrefix);
for (auto item : m_caches)
- item->save(m_localPrefix);
+ saveWithRetry(item);
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Blocking retry loop with QThread::msleep may stall the event loop under load
`saveWithRetry` uses `QThread::msleep` in a loop on the main thread, where `save()` is called. This can block the event loop for hundreds of ms per cache under heavy writes, delaying DBus handling and timers. If retries are required, avoid sleeping on the main thread—e.g. just log and continue, or move retries to a worker thread / queued `QTimer` so the main event loop stays responsive.
Suggested implementation:
```cpp
for (auto item : m_caches)
item->save(m_localPrefix);
}
```
`saveWithRetry` is still in the codebase and currently uses `QThread::msleep` in a loop. To fully address the concern:
1. Either remove `saveWithRetry` if no longer needed, or
2. Refactor it to run in a worker thread or use a `QTimer`/queued connection so any retries are asynchronous and do not block the main event loop.
You’ll also want to update any other call sites of `saveWithRetry` (if they exist) to ensure they don’t block the main thread in the same way.
</issue_to_address>
### Comment 6
<location path="dconfig-center/tests/ut_dconfigresource.cpp" line_range="59-68" />
<code_context>
+ QScopedPointer<DSGConfigServer> server;
+};
+
+// T11-1a: reparse() 调用后值能被正确更新（仅测试调用不崩溃）
+TEST_F(ut_DSGConfigResource, test_reparse)
+{
+ // 通过 server 拿到 resource
+ auto path = server->acquireManager(APP_ID, FILE_NAME, "");
+ ASSERT_FALSE(path.path().isEmpty());
+
+ auto resource = server->resourceObject(getGenericResourceKey(FILE_NAME, QString()));
+ if (!resource) {
+ GTEST_SKIP() << "resource not found, skip reparse test";
+ }
+ // reparse 不应崩溃
+ EXPECT_NO_FATAL_FAILURE(resource->reparse(APP_ID));
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** reparse() test only checks that it does not crash but does not verify that parsing actually updates values
Since the comment says "值能被正确更新", this test should assert at least one actual value change, not just that it doesn’t crash. For example, modify the underlying JSON (or use two variants) before calling `reparse(APP_ID)`, then verify that `resource` reads the updated value. Alternatively, read a known key before and after `reparse()` and assert the expected change to prove the reload path really works.
</issue_to_address>
### Comment 7
<location path="dconfig-center/dde-dconfig-daemon/main.cpp" line_range="104" />
<code_context>
Dtk::Core::DLogManager::registerFileAppender();
qInfo() << "Log path is:" << Dtk::Core::DLogManager::getlogFilePath();
+
+ // T01-2：POSIX 信号 → Qt 事件桥（有序关闭）
+ if (setupSignalBridge()) {
+ auto *signalNotifier = new QSocketNotifier(g_signalFd[1], QSocketNotifier::Read, &a);
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing all shutdown behavior in the aboutToQuit handler so that signal handling only requests application quit and does not duplicate cleanup or lifecycle transitions.
You can keep all new functionality (ordered shutdown, logging, lifecycle) but simplify by centralizing shutdown logic and making the signal bridge only responsible for *requesting* a quit.
Right now you have:
- `dsgConfig.exit()` called in both the signal-notifier lambda and the `aboutToQuit` handler.
- `ServiceLifecycle` transitions duplicated in the signal-notifier lambda instead of being centralized.
- Two different shutdown code paths (bridge vs fallback) that behave differently.
You can reduce complexity by:
1. Letting the POSIX → Qt bridge only call `QCoreApplication::quit()` and log the signal.
2. Doing **all** cleanup (lifecycle transitions + `dsgConfig.exit()`) only in `aboutToQuit`.
3. Making the fallback signal handler mirror the bridge behavior (log + quit only).
This keeps the bridge and the lifecycle, but the entry point becomes much easier to reason about: “signals → quit request; aboutToQuit → cleanup”.
### Suggested refactor
**Signal bridge: only request quit**
```cpp
if (setupSignalBridge()) {
auto *signalNotifier = new QSocketNotifier(g_signalFd[1],
QSocketNotifier::Read,
&a);
QObject::connect(signalNotifier, &QSocketNotifier::activated, &a,
[](int) {
char sig = 0;
::read(g_signalFd[1], &sig, 1);
qInfo("[main] Received signal %d, requesting shutdown...",
static_cast<int>(sig));
QCoreApplication::quit();
});
} else {
// 降级：直接用旧的 signal handler（也只触发退出）
struct sigaction sa;
sa.sa_handler = [](int sig) {
qInfo("Received signal %d, requesting shutdown.", sig);
QCoreApplication::quit();
};
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESETHAND;
sigaction(SIGTERM, &sa, nullptr);
sigaction(SIGINT, &sa, nullptr);
}
```
**Centralize lifecycle + cleanup in one place**
```cpp
QObject::connect(qApp, &QCoreApplication::aboutToQuit,
[&lifecycle, &dsgConfig]() {
qInfo() << "[main] Exit dconfig daemon and release resources.";
lifecycle.transitionTo(ServiceState::Stopping);
dsgConfig.exit();
lifecycle.transitionTo(ServiceState::Stopped);
});
```
This way:
- There is a **single, canonical shutdown path** (`aboutToQuit`).
- Signal handling is minimal and consistent across bridge/fallback.
- `ServiceLifecycle` actually *centralizes* lifecycle transitions instead of being partially driven from the signal handler.
- You avoid double-calling `dsgConfig.exit()` and double-logging shutdown.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+52
| /// T06-4: 配置文件更新时通知调用方（appid/resource 来自路径解析） | ||
| void configUpdated(const QString &appid, const QString &resource); |
There was a problem hiding this comment.
issue (bug_risk): configUpdated should be declared as a Qt signal if it is emitted
In dconfigserver.cpp, configUpdated is emitted (emit configUpdated(...)) but is declared here as a regular method instead of a signal. This will cause a compilation error or prevent it from working as a Qt signal. Please move this declaration into a Q_SIGNALS: section so emit is valid and it can be connected from other objects.
Comment on lines
+10
to
+12
| #ifdef Q_OS_LINUX | ||
| #include <sys/inotify.h> | ||
| #include <unistd.h> |
There was a problem hiding this comment.
issue (bug_risk): Missing errno/strerror includes for Linux branch
errno and strerror(errno) are used in the Linux code paths, but the file doesn’t include <errno.h> or <string.h>/<cstring>. Please add these headers explicitly rather than relying on transitive includes, which are undefined and may fail on some toolchains.
Comment on lines
+28
to
+33
| const QString abs = m_localPrefix + path; | ||
| // 避免重复 | ||
| for (const auto &p : m_paths) { | ||
| if (p.second == abs) return; | ||
| } | ||
| m_paths.append({priority, abs}); |
There was a problem hiding this comment.
issue (bug_risk): addSearchPath unconditionally prefixes with m_localPrefix, which can double-prefix or break absolute paths
Here path is always prefixed with m_localPrefix, but call sites already pass absolute paths (e.g. "/usr/share/dsg/configs") and linglongPath is validated via QDir(m_localPrefix + linglongPath).exists() before being passed in. As a result, you can end up storing m_localPrefix + m_localPrefix + linglongPath, and absolute paths in general get incorrectly prefixed. Consider checking whether path is relative and only then prepending m_localPrefix (e.g. using QDir::isRelativePath(path)).
Comment on lines
+72
to
+73
| if (!m_localPrefix.isEmpty() || true) { | ||
| result << QString("%1/etc/dsg/configs/overrides/%2/%3.json") |
There was a problem hiding this comment.
issue (bug_risk): Condition if (!m_localPrefix.isEmpty() || true) is always true and likely not intended
Because of the || true, this block always executes and the /etc/dsg/configs/overrides/... path is always added, making the m_localPrefix.isEmpty() check pointless and suggesting a leftover from debugging. If this path should only be added when a local prefix exists, drop || true; if it should always be added, remove the if entirely for clarity.
Comment on lines
+454
to
+459
| qDebug(cfLog, "Save resource's cache for [%s], and cache count:%lld", qPrintable(m_key), m_caches.count()); | ||
| for (auto item : m_files) | ||
| item->save(m_localPrefix); | ||
|
|
||
| for (auto item : m_caches) | ||
| item->save(m_localPrefix); | ||
| saveWithRetry(item); |
There was a problem hiding this comment.
suggestion (bug_risk): Blocking retry loop with QThread::msleep may stall the event loop under load
saveWithRetry uses QThread::msleep in a loop on the main thread, where save() is called. This can block the event loop for hundreds of ms per cache under heavy writes, delaying DBus handling and timers. If retries are required, avoid sleeping on the main thread—e.g. just log and continue, or move retries to a worker thread / queued QTimer so the main event loop stays responsive.
Suggested implementation:
for (auto item : m_caches)
item->save(m_localPrefix);
}
saveWithRetry is still in the codebase and currently uses QThread::msleep in a loop. To fully address the concern:
- Either remove
saveWithRetryif no longer needed, or - Refactor it to run in a worker thread or use a
QTimer/queued connection so any retries are asynchronous and do not block the main event loop.
You’ll also want to update any other call sites ofsaveWithRetry(if they exist) to ensure they don’t block the main thread in the same way.
Comment on lines
+59
to
+68
| // T11-1a: reparse() 调用后值能被正确更新（仅测试调用不崩溃） | ||
| TEST_F(ut_DSGConfigResource, test_reparse) | ||
| { | ||
| // 通过 server 拿到 resource | ||
| auto path = server->acquireManager(APP_ID, FILE_NAME, ""); | ||
| ASSERT_FALSE(path.path().isEmpty()); | ||
|
|
||
| auto resource = server->resourceObject(getGenericResourceKey(FILE_NAME, QString())); | ||
| if (!resource) { | ||
| GTEST_SKIP() << "resource not found, skip reparse test"; |
There was a problem hiding this comment.
suggestion (testing): reparse() test only checks that it does not crash but does not verify that parsing actually updates values
Since the comment says "值能被正确更新", this test should assert at least one actual value change, not just that it doesn’t crash. For example, modify the underlying JSON (or use two variants) before calling reparse(APP_ID), then verify that resource reads the updated value. Alternatively, read a known key before and after reparse() and assert the expected change to prove the reload path really works.
| Dtk::Core::DLogManager::registerFileAppender(); | ||
| qInfo() << "Log path is:" << Dtk::Core::DLogManager::getlogFilePath(); | ||
|
|
||
| // T01-2：POSIX 信号 → Qt 事件桥（有序关闭） |
There was a problem hiding this comment.
issue (complexity): Consider centralizing all shutdown behavior in the aboutToQuit handler so that signal handling only requests application quit and does not duplicate cleanup or lifecycle transitions.
You can keep all new functionality (ordered shutdown, logging, lifecycle) but simplify by centralizing shutdown logic and making the signal bridge only responsible for requesting a quit.
Right now you have:
dsgConfig.exit()called in both the signal-notifier lambda and theaboutToQuithandler.ServiceLifecycletransitions duplicated in the signal-notifier lambda instead of being centralized.- Two different shutdown code paths (bridge vs fallback) that behave differently.
You can reduce complexity by:
- Letting the POSIX → Qt bridge only call
QCoreApplication::quit()and log the signal. - Doing all cleanup (lifecycle transitions +
dsgConfig.exit()) only inaboutToQuit. - Making the fallback signal handler mirror the bridge behavior (log + quit only).
This keeps the bridge and the lifecycle, but the entry point becomes much easier to reason about: “signals → quit request; aboutToQuit → cleanup”.
Suggested refactor
Signal bridge: only request quit
if (setupSignalBridge()) {
auto *signalNotifier = new QSocketNotifier(g_signalFd[1],
QSocketNotifier::Read,
&a);
QObject::connect(signalNotifier, &QSocketNotifier::activated, &a,
[](int) {
char sig = 0;
::read(g_signalFd[1], &sig, 1);
qInfo("[main] Received signal %d, requesting shutdown...",
static_cast<int>(sig));
QCoreApplication::quit();
});
} else {
// 降级：直接用旧的 signal handler（也只触发退出）
struct sigaction sa;
sa.sa_handler = [](int sig) {
qInfo("Received signal %d, requesting shutdown.", sig);
QCoreApplication::quit();
};
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESETHAND;
sigaction(SIGTERM, &sa, nullptr);
sigaction(SIGINT, &sa, nullptr);
}Centralize lifecycle + cleanup in one place
QObject::connect(qApp, &QCoreApplication::aboutToQuit,
[&lifecycle, &dsgConfig]() {
qInfo() << "[main] Exit dconfig daemon and release resources.";
lifecycle.transitionTo(ServiceState::Stopping);
dsgConfig.exit();
lifecycle.transitionTo(ServiceState::Stopped);
});This way:
- There is a single, canonical shutdown path (
aboutToQuit). - Signal handling is minimal and consistent across bridge/fallback.
ServiceLifecycleactually centralizes lifecycle transitions instead of being partially driven from the signal handler.- You avoid double-calling
dsgConfig.exit()and double-logging shutdown.
Contributor
|
TAG Bot New tag: 1.0.42 |
Contributor
|
TAG Bot New tag: 1.0.43 |
Contributor
|
TAG Bot New tag: 1.0.44 |
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
2 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.
概述
本 PR 是对
dde-dconfig-daemon的系统性重构，按 P0→P1→P2 优先级分阶段实现，所有改动已整合到一个分支，编译通过。P0：核心稳定性
T04 — 连接管理与引用计数优化
ConnKey类型别名，统一连接标识格式DSGConfigRefManager改为全局单定时器（60s）+QHash存储，降低定时器开销[leak]警告日志T05 — 写盘策略重构
ConfigSyncPolicy接口（sync()/flush()）DelayedSyncPolicy：500ms 防抖延迟 + 最大 5s 强制写盘上限flush()确保数据落地T06 — inotify 驱动热重载
InotifyWatcher类，替代旧的文件签名轮询方案configUpdated(path)信号触发 reload，减少无效 I/OP1：服务框架现代化
T01 — 服务生命周期 & 信号处理
ServiceLifecycle状态机（Initializing→Running→Stopping→Stopped）main.cpp使用socketpair桥接 POSIX 信号（SIGTERM/SIGINT）到 Qt 事件，async-signal-safeQT_MESSAGE_PATTERN含时间戳 + PID + categoryT07 — 安全增强
removeUserData()添加[AUDIT]级别日志，记录 uid/caller/pidacquireManagerV2()添加 uid 校验，非 root 调用方不能跨 uid 访问T03 — 路径管理
ConfigPathResolver单例，支持优先级排序initialize()通过 resolver 统一注册路径（etc=200 > usr=100 > linglong=50）P2：工具增强与测试
T09 — CLI 增强（
dde-dconfig）--output json|text：list/get 支持 JSON 输出watch命令支持 JSON 格式（含 key/appid/resource）export子命令：将全量配置值导出为 JSON 快照文件import子命令：从 JSON 快照批量导入配置值T08 — 配置格式扩展
value()：检测 description 中的[deprecated]标记，发出qCWarningsetValue()：解析 description 中的[schema:{type,min,max}]标记，类型/范围校验失败返回org.desktopspec.ConfigManager.InvalidValueT11 — 单元测试覆盖率
ut_dconfigresource.cpp：覆盖ServiceLifecycle状态机转换、ConfigPathResolver优先级/去重/路径候选、reparse 调用.github/workflows/test.yml：CI 自动构建 +ctest+gcovr覆盖率报告测试方法
文件变更
dconfigserver.cpp/hdconfigconn.cppconfigsyncpolicy.hinotifywatcher.cpp/hservicelifecycle.cpp/hconfigpathresolver.cpp/hdde-dconfig/main.cpptests/ut_dconfigresource.cpp.github/workflows/test.ymlSummary by Sourcery
Refactor dde-dconfig-daemon’s core service, sync pipeline, and CLI to improve reliability, observability, and configurability while adding JSON-based tooling and tests.
New Features:
Enhancements:
CI:
Tests: