Repository navigation
迁移配置至 dconfig - #109
Draft
glyvut wants to merge 1 commit into
Draft
迁移配置至 dconfig#109glyvut wants to merge 1 commit into
glyvut wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: glyvut 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 |
|
TAG Bot TAG: 0.4.1 |
Reviewer's GuideMigrates DDM’s main and state settings from layered INI files to typed DConfig schemas installed in the shared DSG directory, adds runtime-safe initialization before daemon startup, and updates build dependencies, packaging, paths, version metadata, and documentation. Sequence diagram for DConfig initialization before daemon startupsequenceDiagram
participant Main as Daemon main
participant App as DaemonApp
participant MainConfig as mainConfig
participant StateConfig as stateConfig
participant DConfig as DConfig
Main->>App: DaemonApp(argc, argv)
App-->>Main: QCoreApplication available
Main->>MainConfig: load()
MainConfig->>DConfig: create(org.deepin.dde.ddm, org.deepin.dde.ddm)
DConfig-->>MainConfig: typed configuration values
Main->>StateConfig: load()
StateConfig->>DConfig: create(org.deepin.dde.ddm, org.deepin.dde.ddm.state)
DConfig-->>StateConfig: persisted state values
Main->>App: exec()
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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/common/Configuration.cpp" line_range="37" />
<code_context>
- auto it = m_entries.find(name);
- if (it != m_entries.end())
- return it.value();
- return nullptr;
- }
-
</code_context>
<issue_to_address>
**issue (bug_risk):** When either DConfig instance cannot be created, `createConfig` returns `nullptr`, but `initConfiguration` leaves the corresponding global null and the daemon later dereferences it through calls such as `mainConfig->autologinUser()`, causing a startup or first-request crash.
**Triggers:** When the dconfig daemon is unavailable or configuration creation fails.
**Suggested fix:** Treat initialization failure as fatal with a clear error and exit, or keep the daemon disabled until both configuration pointers are valid.
</issue_to_address>
### Comment 2
<location path="src/common/Configuration.cpp" line_range="48" />
<code_context>
+ return nullptr;
+
+ if (config->isInitializeSucceeded())
+ return config;
+
+ QEventLoop loop;
</code_context>
<issue_to_address>
**issue (bug_risk):** After `configInitializeFailed` or the five-second timeout, `createConfig` still returns the uninitialized configuration object instead of reporting failure, so callers read and write an object whose DConfig state was never initialized.
**Triggers:** When asynchronous DConfig initialization fails or does not complete within five seconds.
**Suggested fix:** Track whether initialization succeeded and return `nullptr` or otherwise fail initialization when the failure signal or timeout ends the event loop.
```suggestion
return config->isInitializeSucceeded() ? config : nullptr;
```
</issue_to_address>
### Comment 3
<location path="src/daemon/DaemonApp.cpp" line_range="123-124" />
<code_context>
std::cout << "Usage: ddm [options]\n"
<< "Options: \n"
- << " --example-config Print the complete current configuration to stdout" << std::endl;
+ << " --test-mode Start daemon in test mode" << std::endl;
return EXIT_FAILURE;
</code_context>
<issue_to_address>
**issue (bug_risk):** The help text advertises `--test-mode`, but `main` only checks `--help` and never parses or applies `--test-mode`; invoking it starts the normal daemon rather than test mode.
**Triggers:** When an operator or test invokes ddm with `--test-mode`.
**Suggested fix:** Implement the test-mode branch before constructing the normal daemon, or remove the option from the help output.
```suggestion
<< "Options: \n";
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
glyvut
marked this pull request as draft
August 27, 2026 06:26
glyvut
marked this pull request as ready for review
August 27, 2026 08:17
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/common/ConfigReader.cpp" line_range="2" />
<code_context>
/*
- * INI Configuration parser classes
+ * INI Configuration parser classes (DConfig backed)
* Copyright (C) 2014 Martin Bříza <mbriza@redhat.com>
*
</code_context>
<issue_to_address>
**nitpick:** The file header still describes these classes as an INI configuration parser even though the implementation no longer reads or writes INI files and instead uses DConfig. This misleads maintainers about the storage backend and the behavior of ConfigBase.
**Suggested fix:** Update the header comments in both files to describe the DConfig-backed configuration wrapper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
glyvut
force-pushed
the
master
branch
2 times, most recently
from
August 27, 2026 08:39
c6ba540 to
3de45e4
Compare
glyvut
marked this pull request as draft
August 27, 2026 08:50
glyvut
marked this pull request as ready for review
August 27, 2026 08:52
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="CMakeLists.txt" line_range="120-124" />
<code_context>
-set(CONFIG_FILE "${CMAKE_INSTALL_FULL_SYSCONFDIR}/ddm.conf" CACHE PATH "Path of the ddm config file")
</code_context>
<issue_to_address>
**issue:** The CMake cache variables `CONFIG_FILE`, `CONFIG_DIR`, and `SYSTEM_CONFIG_DIR` are removed while the man-page templates still reference them during configuration. The generated `ddm(1)`, `ddm.conf(5)`, and `ddm-state.conf(5)` documentation therefore contains empty configuration paths instead of describing the DConfig storage locations.
**Triggers:** When `BUILD_MAN_PAGES` is enabled.
**Suggested fix:** Update the man-page templates and their configure inputs to document the DConfig schema and state locations, or retain suitable path variables for the generated documentation.
</issue_to_address>
### Comment 2
<location path="src/common/ConfigReader.cpp" line_range="92-100" />
<code_context>
+ return key;
+ }
+
+ DTK_CORE_NAMESPACE::DConfig *dconfigFor(const QString &appId, const QString &name) {
+ static QHash<QString, DTK_CORE_NAMESPACE::DConfig *> s_configs;
+ const QString cacheKey = appId + QLatin1Char('/') + name;
+ auto it = s_configs.constFind(cacheKey);
+ if (it != s_configs.constEnd())
+ return it.value();
+ auto *config = DTK_CORE_NAMESPACE::DConfig::create(appId, name, QString(), nullptr);
+ s_configs.insert(cacheKey, config);
+ return config;
+ }
+
</code_context>
<issue_to_address>
**nitpick (bug_risk):** Every DConfig object created by `dconfigFor` is allocated with a null parent and retained in a process-global `QHash` without any ownership or cleanup. Each distinct app/name pair permanently leaks its DConfig object for the lifetime of the process.
**Triggers:** When additional distinct DConfig configurations are requested in a long-running process.
**Suggested fix:** Give the objects an owning Qt parent or store them in an owning smart pointer/container and release them during 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.
|
TAG Bot New tag: 0.4.0 |
glyvut
force-pushed
the
master
branch
2 times, most recently
from
October 12, 2026 08:51
637b4c5 to
ad2fdeb
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Global writable autologin settings, missing legacy-value migration, and DConfig packaging/interface defects must be addressed. / 必须解决全局可写自动登录、旧配置迁移缺失及 DConfig 打包接口问题。
7 open findings
What changed in this PR
Migrates DDM configuration and session state from INI files to DConfig. / 将 DDM 配置和会话状态从 INI 迁移至 DConfig。
Changes:
- Adds DConfig-backed loading, saving, and schemas. / 新增 DConfig 读写及 schema。
- Updates build dependencies, packaging, and version to 0.4.1. / 更新构建依赖、打包及版本。
- Revises configuration documentation. / 更新配置文档。
| File | Description |
|---|---|
CMakeLists.txt |
Adds DTK/DConfig setup and version bump. / 添加 DTK/DConfig 配置并升级版本。 |
.github/workflows/ddm-archlinux-build.yml |
Adds Arch DTK dependency. / 添加 Arch DTK 依赖。 |
debian/changelog |
Records release changes. / 记录发行变更。 |
debian/control |
Adds Debian DTK dependencies. / 添加 Debian DTK 依赖。 |
services/ddm-tmpfiles.conf.in |
Removes obsolete state-file note. / 移除过时状态文件说明。 |
src/common/CMakeLists.txt |
Links DTK Core. / 链接 DTK Core。 |
src/common/Constants.h.in |
Removes INI path constants. / 移除 INI 路径常量。 |
src/common/Configuration.h |
Defines DConfig resources. / 定义 DConfig 资源。 |
src/common/ConfigReader.cpp |
Replaces INI persistence with DConfig. / 使用 DConfig 替代 INI 持久化。 |
src/daemon/DaemonApp.cpp |
Reloads configuration after application creation. / 应用创建后重新加载配置。 |
data/CMakeLists.txt |
Installs DConfig schemas. / 安装 DConfig schema。 |
data/dconfig/org.deepin.dde.ddm.json.in |
Defines main configuration schema. / 定义主配置 schema。 |
data/dconfig/org.deepin.dde.ddm.state.json |
Defines state schema. / 定义状态 schema。 |
data/man/ddm.rst.in |
Documents DConfig files. / 说明 DConfig 文件。 |
data/man/ddm.conf.rst.in |
Documents migrated settings. / 说明迁移后的配置项。 |
data/man/ddm-state.conf.rst.in |
Documents state resource. / 说明状态资源。 |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "name[zh_CN]": "关机命令", | ||
| "description": "Halt command.", | ||
| "description[zh_CN]": "关机命令。", | ||
| "permissions": "read", |
glyvut
marked this pull request as draft
October 13, 2026 00:49
glyvut
marked this pull request as ready for review
October 13, 2026 13:10
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/common/Configuration.h" line_range="83" />
<code_context>
);
- Config(StateConfig, []()->QString{auto tmp = getpwnam("ddm"); return tmp ? QString::fromLocal8Bit(tmp->pw_dir) : QStringLiteral(STATE_DIR);}().append(QStringLiteral("/state.conf")), QString(), QString(),
+ Config(StateConfig, _S(DDM_DCONFIG_APP_ID ".state"), _S(DDM_DCONFIG_APP_ID), QString(),
Section(Last,
Entry(Session, QString, QString(), _S("Name of the session for the last logged-in user.\n"
</code_context>
<issue_to_address>
**Writable session state runs attacker code**
When a local user sets `lastSession` to a desktop file they control and DDM uses the remembered session for configured autologin, `stateConfig.Last.Session` is accepted from DConfig and passed to `Session::setTo()`, where an absolute path bypasses the configured session directories; DDM parses that file's `Exec` entry and launches it during passwordless autologin, running the attacker's command as the autologin user.
Restrict writes to the state key and reject session paths that do not resolve to files inside the configured, trusted session directories.
</issue_to_address>
### Comment 2
<location path="CMakeLists.txt" line_range="87" />
<code_context>
qt_standard_project_setup(REQUIRES 6.6)
+# DTK (DConfig)
+find_package(Dtk6 REQUIRED COMPONENTS Core)
+
# Uninstall target
</code_context>
<issue_to_address>
**Nix package fails to configure**
When the Nix package is built, `find_package(Dtk6 REQUIRED COMPONENTS Core)` cannot find Dtk6 in the Nix build environment, so CMake configuration fails before the package can build.
Add the Nix package providing Dtk6 Core to `nix/default.nix` build inputs.
</issue_to_address>
### Comment 3
<location path="src/common/ConfigReader.cpp" line_range="247" />
<code_context>
+ if (e->matchesDefault())
+ config->reset(key);
+ else
+ config->setValue(key, stringToVariant(config, key, e->value()));
};
</code_context>
<issue_to_address>
**Most settings stay read-only**
When an administrator tries to change a non-autologin setting through DConfig, the main schema marks those settings as read-only, so DConfig rejects the updates and administrators cannot configure settings that were writable through the previous INI files.
Mark the non-autologin settings that administrators are meant to configure as writable in the schema.
</issue_to_address>
### Comment 4
<location path="src/common/ConfigReader.cpp" line_range="105-114" />
<code_context>
+ return config.data();
+ }
+
+ QString variantToString(const QVariant &value) {
+ if (value.typeId() == QMetaType::QStringList)
+ return value.toStringList().join(QLatin1Char(','));
+ if (value.typeId() == QMetaType::QVariantList) {
+ QStringList stringList;
+ const auto list = value.toList();
+ for (const QVariant &item : list)
+ stringList << item.toString();
+ return stringList.join(QLatin1Char(','));
+ }
+ return value.toString();
+ }
</code_context>
<issue_to_address>
**Comma-containing paths are split**
When a DConfig list element, such as a namespace path, contains a comma, `variantToString` joins DConfig list elements with commas, and `ConfigEntry::setValue` splits that string on commas, so a single path containing a comma becomes multiple paths. `UserSession::childModifier` then fails to open a namespace path and aborts session startup.
Preserve list elements directly when loading DConfig arrays instead of serializing and reparsing them as comma-separated text.
</issue_to_address>
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： 建议在 save() 方法中增加 config->isValid() 检查，与 load() 保持一致。 2. 代码质量 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： 在 Configuration.h 文件末尾添加一个换行符。 3. 代码性能 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： DConfig 实例通过静态 QHash 缓存，避免了重复创建。load() 和 save() 方法从原来的文件 I/O 操作改为 DConfig D-Bus 调用，代码大幅简化，性能合理。 4. 代码安全 🔒评价： 优秀 ✅ 通过
安全漏洞详情： 建议： DConfig 框架通过 D-Bus 守护进程管理配置，比原始 INI 文件方式更安全。DConfig schema 中大多数配置项设置了 permissions: read 和 visibility: private，仅 autologinUser 和 autologinSession 设置为 readwrite/public（功能需要）。无硬编码密钥、无命令注入风险、无敏感信息泄露。 💡 改进建议代码示例// 暂无代码示例本报告由 AI 代码审查工具自动生成 |
wineee
reviewed
Oct 14, 2026
wineee
reviewed
Oct 14, 2026
glyvut
marked this pull request as draft
October 19, 2026 06:19
glyvut
force-pushed
the
master
branch
6 times, most recently
from
October 19, 2026 08:12
8468527 to
0d456ff
Compare
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
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
将 DDM 的配置管理迁移到 DConfig，并调整会话发现、登录协议和启动流程以配合新的配置来源。
New Features:
Enhancements:
Build:
Documentation:
Chores: