Repository navigation
迁移配置至 dconfig - #109
Merged
Merged
迁移配置至 dconfig#109
Conversation
|
TAG Bot TAG: 0.4.1 |
Reviewer's GuideMigrates DDM from legacy configuration and daemon-managed login state to the shared DConfig schema, with asynchronous daemon initialization, greeter-owned session selection/history, and explicit propagation of session settings to a configuration-free PAM helper; build, packaging, and documentation are updated accordingly. Sequence diagram for DConfig initialization and greeter session selectionsequenceDiagram
participant DConfig
participant Daemon
participant Greeter
participant Session
Daemon->>DConfig: DdmConfig::create()
DConfig-->>Daemon: configInitializeSucceed
Daemon->>Greeter: SessionDirs(waylandSessionDir, x11SessionDir)
Greeter->>Daemon: Login(user, password, absoluteSessionPath)
Daemon->>Session: Session(sessionPath)
Session-->>Daemon: validated session
Sequence diagram for configuration propagation to the PAM helpersequenceDiagram
participant Daemon
participant Helper
participant PAM
participant UserSession
Daemon->>Helper: openSession(env, cookie, display, tty, type, command, sessionCommand, sessionLogFile, namespaces)
Helper->>PAM: authenticate and open_session
PAM-->>Helper: session opened
Helper->>UserSession: setSessionCommand(sessionCommand)
Helper->>UserSession: setSessionLogFile(sessionLogFile)
Helper->>UserSession: setNamespaces(namespaces)
UserSession->>UserSession: start(command, type, cookie)
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.
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>
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
3 times, most recently
from
October 19, 2026 06:54
0fa68b6 to
ebe8ccc
Compare
glyvut
force-pushed
the
master
branch
11 times, most recently
from
October 20, 2026 02:59
1fc1ea9 to
fd2985e
Compare
glyvut
marked this pull request as ready for review
October 20, 2026 03:01
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="CMakeLists.txt" line_range="87" />
<code_context>
qt_standard_project_setup(REQUIRES 6.6)
+# DTK (DConfig)
+find_package(Dtk6 REQUIRED COMPONENTS Core Tools)
+
# Uninstall target
</code_context>
<issue_to_address>
**Nix builds lack DTK dependencies**
When DDM is built through `nix/default.nix`, `find_package(Dtk6 REQUIRED COMPONENTS Core Tools)` fails during CMake configuration in the Nix derivation because neither its `buildInputs` nor `nativeBuildInputs` includes DTK6. Nix builds of DDM therefore cannot configure.
Add the DTK6 Core and Tools packages to the Nix derivation’s appropriate inputs.
</issue_to_address>
wineee
reviewed
Oct 20, 2026
wineee
reviewed
Oct 20, 2026
wineee
reviewed
Oct 20, 2026
glyvut
force-pushed
the
master
branch
2 times, most recently
from
October 20, 2026 05:10
8266fab to
6838e30
Compare
wineee
previously approved these changes
Oct 20, 2026
Session, LogindDBusTypes and MessageHandler are used by the ddm daemon only. Keeping them in the shared library made the session helper link against daemon-only code, and SocketWriter exposed Session even though it never writes one. 将 Session、LogindDBusTypes 与 MessageHandler 移到 daemon：它们只被 ddm 使用，留在共享库里会让 session helper 链接 daemon 专用代码；同时去掉 SocketWriter 中从未使用的 Session 序列化。 Log: libddm-common 不再包含 daemon 专用实现 PMS: BUG-294419 Influence: 无行为变化，libddm-common 导出的头文件减少；需重新构建依赖它的模块。
Replace the INI configuration with the generated DConfig schema org.deepin.dde.ddm. The daemon waits for the asynchronous configuration before starting the seat manager, and passes the session command, log file and namespaces to ddm-helper through openSession instead of creating a DConfig object before pam_open_session() forks. 用生成的 DConfig schema 替代 INI 配置。daemon 等待异步配置就绪后再 启动 seat manager；会话命令、日志文件与命名空间改由 openSession 传给 ddm-helper，避免在 pam_open_session() fork 前创建 DConfig。 DDM also owns the session directories and the last login: it sends SessionDirs and LastLogin to the greeter, records lastUser/lastSession on successful login, and falls back to lastSession when autologinSession is empty. The installed schema is the reference for keys and defaults; --example-config is removed and DTK6 is now required. 会话目录与上次登录也归 DDM：向 greeter 下发 SessionDirs 与 LastLogin， 登录成功后记录 lastUser/lastSession，autologinSession 为空时回退到 lastSession。schema 即配置参考，删除 --example-config，构建新增 DTK6 依赖。 Log: DDM 配置迁移到 DConfig，并接管会话目录与上次登录状态 PMS: BUG-294419 Influence: 不再读取 /etc/ddm.conf；需安装 DSG schema；新增 SessionDirs/LastLogin 消息，Login 改为传 desktop 文件绝对路径，需与 treeland 同步升级；删除 state.conf、RememberLastUser、 RememberLastSession 与 --example-config。
Describe the DConfig schema as the configuration reference and document the SessionDirs/LastLogin messages and the desktop file path in Login. Drop the obsolete ddm.conf(5) page; ddm(1) now covers the schema, dde-dconfig usage and the DDM-owned last login state. 以 schema 作为配置参考，补充 SessionDirs/LastLogin 消息与 Login 的 desktop 文件路径；删除 ddm.conf(5)，并在 ddm(1) 中说明 schema、 dde-dconfig 用法及 DDM 维护的上次登录状态。 Log: 文档与 DConfig 配置及新的登录流程保持一致 PMS: BUG-294419 Influence: 删除 ddm.conf(5)，配置项以 schema 为准。
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 语法正确，逻辑清晰 ✅ 通过 潜在问题： 建议： ['建议在 recordLastLogin() 方法注释中说明：相比旧版本，RememberLastUser/RememberLastSession 选项已被移除，现在始终记录最后登录用户和会话。'] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： [] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： [] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： [] 💡 改进建议代码示例本次提交无需修复，代码质量良好。以下是一个可选的改进建议——在 recordLastLogin() 方法中添加注释说明行为变化： /**
* Remember the user and optionally the session of a successful
* login. ddm owns this state and hands it to the greeter.
*
* Note: Unlike the previous INI-based configuration, this always
* records the last user and session. The RememberLastUser and
* RememberLastSession options have been removed in the DConfig
* migration; if selective recording is needed again, it should
* be added as a DConfig option.
*/
void Display::recordLastLogin(const QString &user, const Session *session) {
mainConfig()->setLastUser(user);
if (session)
mainConfig()->setLastSession(session->fileName());
}本报告由 AI 代码审查工具自动生成 |
评审意见把配置集中到 DConfig、把会话发现与"上次登录"下放给 greeter，方向是对的。但建议先不要合并：第 1 条是权限模型降级，第 2、3 条会让 daemon 在异常情况下静默失效。行号取自 head P0 · 任意本地用户可改写 root 执行的配置
而 dde-dconfig 侧没有任何跨 uid 授权：
结论：普通用户可以 建议：特权 key 在 schema 中设 P1 · 初始化失败或无响应时，daemon 仍然带着空配置跑起来
反过来，DConfig 一直不回复（dde-dconfig-daemon 卡住）时没有超时，seat manager 永不启动 —— 黑屏且无日志。 建议：初始化失败时 P1 · 报文格式变了，但没有版本协商，包版本也没动
新 ddm + 旧 treeland greeter（或反过来）不会报错，只会表现为"登录没反应 / 会话列表为空"，并且 QDataStream 会读错位。建议在 另外： P2 · 小问题
已核对没问题的部分
|
Contributor
Author
完全不成立，只要 flag 无 user-public 即无权限
完全不成立，初始化失败回落的是默认配置
包版本之后变动，treeland 与 DDM 同步升级 |
Groveer
approved these changes
Oct 21, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: glyvut, Groveer 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
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 及 greeter 协作模型。
New Features:
Enhancements:
Build:
CI:
Documentation:
Chores: