refactor: Refactor config reader & config API #77
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456 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 |
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideRefactors the configuration system by replacing the old ConfigReader/Configuration API with a new unified Config class and updates all call sites, while also switching TTY-in-use detection to systemd sd-login APIs and adjusting example-config generation. Sequence diagram for main configuration usage during daemon startupsequenceDiagram
participant Main as main
participant DaemonApp as DaemonApp
participant Config as mainConfig
Main->>DaemonApp: parseCommandLine()
DaemonApp->>Config: load()
alt load fails
DaemonApp->>Config: wipe()
DaemonApp->>Config: defaultConfig()
DaemonApp-->>Main: print example config (when --example-config)
else load succeeds
DaemonApp->>Config: get<QString>(Users, DefaultPath)
DaemonApp->>Config: get<bool>(Users, RememberLastUser)
DaemonApp->>Config: get<bool>(Users, RememberLastSession)
DaemonApp-->>Main: proceed with normal startup
end
Class diagram for new unified Config APIclassDiagram
class Config {
- QString m_configPath
- QMap~QString, QMap~QString, QVariant~~ m_data
- QMap~QString, QMap~QString, QPair~QVariant, QString~~~ m_defaults
+ Config(QString configPath, QMap~QString, QMap~QString, QPair~QVariant, QString~~~ defaults, QObject parent)
+ T get~T~(QString section, QString entry) const
+ T get~T~(QString entry) const
+ void set~T~(QString section, QString entry, T value)
+ void set~T~(QString entry, T value)
+ bool setDefault(QString section, QString entry)
+ bool setDefault(QString entry)
+ bool load()
+ bool save() const
+ QString defaultConfig() const
+ void wipe()
}
class mainConfig {
<<singleton>>
}
class stateConfig {
<<singleton>>
}
QObject <|-- Config
mainConfig ..> Config : global_instance
stateConfig ..> Config : global_instance
class ThemeSection {
<<config_section>>
+ CursorTheme : QString
+ CursorSize : QString
}
class X11Section {
<<config_section>>
+ ServerPath : QString
+ ServerArguments : QString
+ SessionDir : QStringList
+ SessionCommand : QString
+ SessionLogFile : QString
+ DisplayCommand : QString
+ DisplayStopCommand : QString
}
class WaylandSection {
<<config_section>>
+ SessionDir : QStringList
+ SessionCommand : QString
+ SessionLogFile : QString
}
class SingleSection {
<<config_section>>
+ SessionCommand : QString
}
class UsersSection {
<<config_section>>
+ DefaultPath : QString
+ RememberLastUser : bool
+ RememberLastSession : bool
}
class RootSection {
<<config_section>>
+ HaltCommand : QString
+ RebootCommand : QString
+ Namespaces : QStringList
}
class LastSection {
<<config_section>>
+ User : QString
+ Session : QString
}
Config "1" o-- "*" ThemeSection : defaults
Config "1" o-- "*" X11Section : defaults
Config "1" o-- "*" WaylandSection : defaults
Config "1" o-- "*" SingleSection : defaults
Config "1" o-- "*" UsersSection : defaults
Config "1" o-- "*" RootSection : defaults
Config "1" o-- "*" LastSection : defaults
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, and left some high level feedback:
- In
isTtyInUse, the RAII guards forsessions,tty, andstatecapture these pointers by value when they are stillnullptr, so the lambdas will not see the updated pointers and will never free the allocated memory; capture them by reference (or wrap them in a small struct) so the cleanup actually runs on the allocated buffers. - The new systemd-based session probing in
isTtyInUsecallssd_get_sessions,sd_session_get_tty, andsd_session_get_statewithout checking their return values or guarding againstnullptrresults (e.g.statepassed tostrcmp), which can lead to undefined behavior on error; add return-code checks and null checks before using these pointers. - The new
Configclass header usesstd::runtime_errorandQTextStreambut does not include<stdexcept>or<QTextStream>, which can cause build or ODR issues on some toolchains; explicitly include these headers where they are used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `isTtyInUse`, the RAII guards for `sessions`, `tty`, and `state` capture these pointers by value when they are still `nullptr`, so the lambdas will not see the updated pointers and will never free the allocated memory; capture them by reference (or wrap them in a small struct) so the cleanup actually runs on the allocated buffers.
- The new systemd-based session probing in `isTtyInUse` calls `sd_get_sessions`, `sd_session_get_tty`, and `sd_session_get_state` without checking their return values or guarding against `nullptr` results (e.g. `state` passed to `strcmp`), which can lead to undefined behavior on error; add return-code checks and null checks before using these pointers.
- The new `Config` class header uses `std::runtime_error` and `QTextStream` but does not include `<stdexcept>` or `<QTextStream>`, which can cause build or ODR issues on some toolchains; explicitly include these headers where they are used.
## Individual Comments
### Comment 1
<location> `src/daemon/Display.cpp:61-70` </location>
<code_context>
- qDebug() << "tty" << desiredTty << "already in use by" << session.user().path.path() << session.state()
- << session.display() << session.desktop() << session.vTNr();
- return true;
+ char **sessions = nullptr;
+ std::unique_ptr<void, std::function<void(void *)>> guard(
+ nullptr,
+ [sessions]([[maybe_unused]] void *) {
+ if (sessions) {
+ for (char **s = sessions; s && *s; ++s)
+ free(*s);
+ free(sessions);
}
+ });
+ sd_get_sessions(&sessions);
+ for (char **s = sessions; s && *s; ++s) {
+ char *tty = nullptr;
</code_context>
<issue_to_address>
**issue (bug_risk):** The RAII guard for `sessions` captures the pointer by value before it is initialized, so it will never free the allocated sessions.
Here `guard` is created while `sessions` is still `nullptr`, and the lambda captures `sessions` by value:
```cpp
char **sessions = nullptr;
std::unique_ptr<void, std::function<void(void *)>> guard(
nullptr,
[sessions]([[maybe_unused]] void *) {
if (sessions) {
for (char **s = sessions; s && *s; ++s)
free(*s);
free(sessions);
}
});
sd_get_sessions(&sessions);
``
Since the capture is by value, the deleter always sees `sessions == nullptr`, so the memory from `sd_get_sessions` is never freed. You can fix this by capturing by reference (`[&sessions](void *) { ... }`) or by using a small RAII type that stores a `char **&` and performs the free logic. The same issue appears for `tty`/`state` in the inner loop.
</issue_to_address>
### Comment 2
<location> `src/daemon/Display.cpp:73-82` </location>
<code_context>
+ });
+ sd_get_sessions(&sessions);
+ for (char **s = sessions; s && *s; ++s) {
+ char *tty = nullptr;
+ char *state = nullptr;
+ std::unique_ptr<void, std::function<void(void *)>> guard2(
+ nullptr,
+ [tty, state]([[maybe_unused]] void *) {
+ if (tty)
+ free(tty);
+ if (state)
+ free(state);
+ });
+ sd_session_get_tty(*s, &tty);
+ sd_session_get_state(*s, &state);
+ if (desiredTty == tty && strcmp(state, "closing") != 0) {
+ qDebug() << "tty" << desiredTty << "already in use by session" << *s;
+ return true;
</code_context>
<issue_to_address>
**issue (bug_risk):** `state` is used with `strcmp` without checking for null and without checking the return codes of the `sd_session_get_*` calls.
Inside the loop, `sd_session_get_tty` and `sd_session_get_state` may fail, leaving `tty`/`state` as `nullptr`, so `strcmp(state, "closing")` invokes undefined behavior and the return codes are ignored.
You can guard against this by checking the return values and only using `tty`/`state` on success, e.g.:
```cpp
if (sd_session_get_tty(*s, &tty) < 0)
continue;
if (sd_session_get_state(*s, &state) < 0)
continue;
if (desiredTty == QLatin1String(tty) && qstrcmp(state, "closing") != 0) {
...
}
```
</issue_to_address>
### Comment 3
<location> `src/daemon/Display.cpp:60` </location>
<code_context>
#define STRINGIFY(x) #x
namespace DDM {
static bool isTtyInUse(const QString &desiredTty) {
- if (Logind::isAvailable()) {
- OrgFreedesktopLogin1ManagerInterface manager(Logind::serviceName(), Logind::managerPath(), QDBusConnection::systemBus());
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `isTtyInUse` to use small typed RAII helpers instead of `std::function`-based `unique_ptr` guards to make ownership clearer and avoid leaks.
You can simplify `isTtyInUse` and remove the `std::function<void(void*)>`/lambda-based guards by using small, typed RAII helpers. That will both reduce complexity and fix a subtle bug with the current capture-by-value of `sessions` (the deleter sees the original `nullptr` and leaks the allocated array).
A concrete refactor could look like this:
```c++
namespace {
using CStr = std::unique_ptr<char, decltype(&free)>;
inline CStr makeCStr(char *p)
{
return CStr(p, &free);
}
inline void freeSessions(char **sessions)
{
if (!sessions)
return;
for (char **s = sessions; *s; ++s)
free(*s);
free(sessions);
}
using SessionList = std::unique_ptr<char *, decltype(&freeSessions)>;
} // namespace
static bool isTtyInUse(const QString &desiredTty)
{
char **rawSessions = nullptr;
if (sd_get_sessions(&rawSessions) < 0 || !rawSessions)
return false;
SessionList sessions(rawSessions, &freeSessions);
for (char **s = sessions.get(); *s; ++s) {
char *rawTty = nullptr;
char *rawState = nullptr;
if (sd_session_get_tty(*s, &rawTty) < 0 || !rawTty)
continue;
if (sd_session_get_state(*s, &rawState) < 0 || !rawState)
continue;
CStr tty = makeCStr(rawTty);
CStr state = makeCStr(rawState);
if (desiredTty == tty.get() && strcmp(state.get(), "closing") != 0) {
qDebug() << "tty" << desiredTty << "already in use by session" << *s;
return true;
}
}
return false;
}
```
Key changes:
- Replace `std::unique_ptr<void, std::function<...>>` with typed aliases (`CStr`, `SessionList`) and simple free functions as deleters.
- Remove nested lifetime guards; resource ownership is explicit and linear.
- Avoid capturing `sessions`, `tty`, `state` in lambdas; the deleters now directly operate on their own pointer.
- Add basic error/null checks for the `sd_*` calls to keep the control flow straightforward and safer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The original ConfigReader is buggy and difficult for maintainence, refactor to make it both simple and keep backward compatibility.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the configuration system by replacing the old ConfigReader/Configuration implementation with a new, simpler Config class. The refactor aims to simplify the codebase while maintaining backward compatibility.
Changes:
- Introduced a new generic
Configclass with template-based get/set methods - Migrated all configuration access across the daemon components to use the new API
- Removed the old
ConfigReaderandConfigurationfiles - Updated CMakeLists.txt to reflect the new configuration interface
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/Config.h | New configuration class implementation with inline global instances |
| src/common/Configuration.h | Deleted old configuration definitions |
| src/common/Configuration.cpp | Deleted old configuration implementation |
| src/common/ConfigReader.h | Deleted old configuration reader interface |
| src/common/ConfigReader.cpp | Deleted old configuration reader implementation |
| src/daemon/XorgDisplayServer.cpp | Updated to use new Config API for X11 server configuration |
| src/daemon/UserSession.cpp | Updated to use new Config API for session commands and namespaces |
| src/daemon/Display.cpp | Updated to use new Config API for user preferences and state management |
| src/daemon/DaemonApp.cpp | Updated to use new defaultConfig() method for --example-config |
| src/daemon/PowerManager.cpp | Updated to use new Config API for halt/reboot commands |
| src/daemon/SeatManager.cpp | Updated include to use new Config.h |
| src/common/Session.cpp | Updated to use new Config API for session directories |
| src/common/XAuth.cpp | Removed unused Configuration.h include |
| src/common/CMakeLists.txt | Updated to expose new Config.h instead of old configuration headers |
| env.insert(QStringLiteral("DISPLAY"), display); | ||
| env.insert(QStringLiteral("HOME"), QStringLiteral("/")); | ||
| env.insert(QStringLiteral("PATH"), mainConfig.Users.DefaultPath.get()); | ||
| env.insert(QStringLiteral("PATH"), mainConfig.get<QString>("Users", "DefaultPath")); |
There was a problem hiding this comment.
Inconsistent indentation detected. This line uses spaces for indentation while surrounding lines use consistent formatting. The indentation should match the surrounding code style.
检测到不一致的缩进。此行使用空格缩进，而周围的行使用一致的格式。缩进应与周围的代码风格匹配。
| } else if (value.indexOf(',') >= 0) { | ||
| QStringList list = value.split(',', Qt::SkipEmptyParts); | ||
| for (QString &item : list) | ||
| item = item.trimmed(); | ||
| m_data[currentSection][name] = QVariant(list); | ||
| } else | ||
| m_data[currentSection][name] = QVariant(value); |
There was a problem hiding this comment.
The load() method treats any value containing a comma as a QStringList (line 141-145), which may incorrectly parse string values that legitimately contain commas but are not intended to be lists. For example, a description like "Hello, world" would be parsed as a list with two elements.
Consider checking the default value's type (if available) to determine whether a value should be parsed as a list, rather than relying solely on the presence of a comma. This would make the parsing more robust and align with the actual data type expectations.
load() 方法将任何包含逗号的值视为 QStringList（第141-145行），这可能会错误解析合法包含逗号但不打算作为列表的字符串值。例如，像 "Hello, world" 这样的描述将被解析为包含两个元素的列表。
建议检查默认值的类型（如果可用）来确定值是否应解析为列表，而不是仅依赖逗号的存在。这将使解析更加健壮，并与实际的数据类型期望保持一致。
| } else if (value.indexOf(',') >= 0) { | |
| QStringList list = value.split(',', Qt::SkipEmptyParts); | |
| for (QString &item : list) | |
| item = item.trimmed(); | |
| m_data[currentSection][name] = QVariant(list); | |
| } else | |
| m_data[currentSection][name] = QVariant(value); | |
| } else { | |
| // Decide whether to treat the value as a list based on the default type. | |
| bool treatAsList = false; | |
| auto sectionDefaultsIt = m_defaults.constFind(currentSection); | |
| if (sectionDefaultsIt != m_defaults.cend()) { | |
| const auto &entryDefaults = sectionDefaultsIt.value(); | |
| auto entryIt = entryDefaults.constFind(name); | |
| if (entryIt != entryDefaults.cend() && | |
| entryIt->first.userType() == QMetaType::QStringList) { | |
| treatAsList = true; | |
| } | |
| } | |
| if (treatAsList) { | |
| QStringList list = value.split(',', Qt::SkipEmptyParts); | |
| for (QString &item : list) | |
| item = item.trimmed(); | |
| m_data[currentSection][name] = QVariant(list); | |
| } else { | |
| m_data[currentSection][name] = QVariant(value); | |
| } | |
| } |
| template<typename T> | ||
| inline T get(const QString &entry) const { | ||
| return get<T>({}, entry); | ||
| } |
There was a problem hiding this comment.
Using an empty string {} to represent the default section is inconsistent with the save() and defaultConfig() methods which explicitly check for non-empty section names (lines 167, 192). This works but makes the code less clear. Consider using a named constant like DEFAULT_SECTION or "General" to improve code readability and maintainability.
使用空字符串 {} 来表示默认部分与 save() 和 defaultConfig() 方法不一致，这些方法明确检查非空部分名称（第167、192行）。这有效但使代码不太清晰。建议使用命名常量，如 DEFAULT_SECTION 或 "General"，以提高代码可读性和可维护性。
| bool load() { | ||
| QFile file(m_configPath); | ||
| if (!file.open(QIODevice::ReadOnly)) { | ||
| return false; | ||
| } | ||
| QString currentSection{}; | ||
| while (!file.atEnd()) { | ||
| QString line = QString::fromLocal8Bit(file.readLine()).trimmed(); | ||
| if (line.isEmpty() || line.startsWith('#')) | ||
| continue; | ||
| if (line.startsWith('[') && line.endsWith(']')) { | ||
| currentSection = line.mid(1, line.length() - 2).trimmed(); | ||
| } else { | ||
| int separatorPosition = line.indexOf('='); | ||
| if (separatorPosition >= 0) { | ||
| if (!m_data.contains(currentSection)) | ||
| m_data[currentSection] = {}; | ||
| QString name = line.left(separatorPosition).trimmed(); | ||
| QString value = line.mid(separatorPosition + 1).trimmed(); | ||
| if (value.isEmpty()) { | ||
| m_data[currentSection][name] = QVariant(); | ||
| } else if (value.indexOf(',') >= 0) { | ||
| QStringList list = value.split(',', Qt::SkipEmptyParts); | ||
| for (QString &item : list) | ||
| item = item.trimmed(); | ||
| m_data[currentSection][name] = QVariant(list); | ||
| } else | ||
| m_data[currentSection][name] = QVariant(value); | ||
| } | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The load() method stores all configuration values as QString (line 147), including boolean values. However, when calling get(), the code attempts to convert QString to bool using QVariant::value(). QVariant's QString to bool conversion doesn't work as expected - it only returns true if the QString can be converted to a number and that number is non-zero, not based on "true"/"false" string values. This will cause boolean configuration values like "RememberLastUser" to always evaluate to false when loaded from a file.
Consider implementing proper type detection and conversion in the load() method. For example, check if a default value exists for the entry and use its type to determine how to parse the string value. Alternatively, implement special parsing for known boolean string values like "true", "false", "yes", "no", "1", "0" (case-insensitive).
boolean 配置值加载存在bug。load() 方法将所有配置值存储为 QString（第147行），包括布尔值。但调用 get() 时，代码尝试使用 QVariant::value() 将 QString 转换为 bool。QVariant 的 QString 到 bool 的转换不是基于 "true"/"false" 字符串，而是检查字符串是否可以转换为非零数字。这将导致从文件加载的布尔配置值（如 "RememberLastUser"）总是评估为 false。
建议在 load() 方法中实现正确的类型检测和转换。例如，检查该条目是否存在默认值，并使用其类型确定如何解析字符串值。或者，为已知的布尔字符串值（如 "true"、"false"、"yes"、"no"、"1"、"0"，不区分大小写）实现特殊解析。
| /** State configuration instance. */ | ||
| inline Config stateConfig( | ||
| QStringLiteral(STATE_DIR "/state.conf"), | ||
| { { "Last", | ||
| { { "User", { QString(), "The last successfully logged in user" } }, | ||
| { "Session", | ||
| { QString(), "The session of the last successfully logged in user" } } } } }); | ||
| } // namespace DDM |
There was a problem hiding this comment.
The inline stateConfig instance is never loaded from the configuration file. While stateConfig.save() is called when user state changes (lines 268, 392 in Display.cpp), there's no corresponding stateConfig.load() call anywhere in the codebase. This means the last user and session state are never restored across daemon restarts, breaking the "remember last user/session" functionality.
Add a stateConfig.load() call during application initialization, likely in DaemonApp constructor or before the first use in Display.cpp.
stateConfig 实例从未从配置文件加载。虽然在用户状态更改时调用了 stateConfig.save()（Display.cpp 的第268、392行），但代码库中没有相应的 stateConfig.load() 调用。这意味着最后一个用户和会话状态在守护进程重启后不会恢复，破坏了"记住上次用户/会话"功能。
建议在应用程序初始化期间添加 stateConfig.load() 调用，可能在 DaemonApp 构造函数中或在 Display.cpp 中首次使用之前。
| bool load() { | ||
| QFile file(m_configPath); | ||
| if (!file.open(QIODevice::ReadOnly)) { | ||
| return false; | ||
| } | ||
| QString currentSection{}; | ||
| while (!file.atEnd()) { | ||
| QString line = QString::fromLocal8Bit(file.readLine()).trimmed(); | ||
| if (line.isEmpty() || line.startsWith('#')) | ||
| continue; | ||
| if (line.startsWith('[') && line.endsWith(']')) { | ||
| currentSection = line.mid(1, line.length() - 2).trimmed(); | ||
| } else { | ||
| int separatorPosition = line.indexOf('='); | ||
| if (separatorPosition >= 0) { | ||
| if (!m_data.contains(currentSection)) | ||
| m_data[currentSection] = {}; | ||
| QString name = line.left(separatorPosition).trimmed(); | ||
| QString value = line.mid(separatorPosition + 1).trimmed(); | ||
| if (value.isEmpty()) { | ||
| m_data[currentSection][name] = QVariant(); | ||
| } else if (value.indexOf(',') >= 0) { | ||
| QStringList list = value.split(',', Qt::SkipEmptyParts); | ||
| for (QString &item : list) | ||
| item = item.trimmed(); | ||
| m_data[currentSection][name] = QVariant(list); | ||
| } else | ||
| m_data[currentSection][name] = QVariant(value); | ||
| } | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The new Config class only loads from a single configuration file (m_configPath), while the old ConfigReader implementation supported loading from multiple configuration directories in order of priority: system config directory (/usr/lib/ddm/ddm.conf.d/), user config directory (/etc/ddm.conf.d/), and the main config file (/etc/ddm.conf). This is a breaking change that removes the ability to use drop-in configuration files, which is a common pattern in system configuration.
Consider restoring support for configuration directories to maintain backward compatibility and allow modular configuration. The old implementation loaded files from configDir and sysConfigDir in alphabetical order before loading the main config file.
新的 Config 类仅从单个配置文件（m_configPath）加载，而旧的 ConfigReader 实现支持按优先级顺序从多个配置目录加载：系统配置目录（/usr/lib/ddm/ddm.conf.d/）、用户配置目录（/etc/ddm.conf.d/）和主配置文件（/etc/ddm.conf）。这是一个破坏性更改，删除了使用插入式配置文件的能力，这是系统配置中的常见模式。
建议恢复对配置目录的支持以保持向后兼容性并允许模块化配置。旧实现在加载主配置文件之前按字母顺序从 configDir 和 sysConfigDir 加载文件。
| m_data[currentSection][name] = QVariant(value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The old ConfigReader implementation had backward compatibility for renamed sections: "XDisplay" was automatically mapped to "X11", and "WaylandDisplay" to "Wayland" during loading (see deleted ConfigReader.cpp lines 210-213). The new Config class doesn't include this backward compatibility, which means existing configuration files using the old section names will not work correctly.
Consider adding backward compatibility mappings in the load() method to handle legacy section names, or document this as a breaking change requiring manual configuration migration.
旧的 ConfigReader 实现对重命名的部分具有向后兼容性："XDisplay" 自动映射到 "X11"，"WaylandDisplay" 映射到 "Wayland"（参见已删除的 ConfigReader.cpp 第210-213行）。新的 Config 类不包含此向后兼容性，这意味着使用旧部分名称的现有配置文件将无法正常工作。
建议在 load() 方法中添加向后兼容性映射以处理旧的部分名称，或将此记录为需要手动配置迁移的破坏性更改。
| } | |
| } | |
| // Backward compatibility for legacy section names. | |
| // Map "XDisplay" -> "X11". | |
| if (m_data.contains(QStringLiteral("XDisplay"))) { | |
| auto legacy = m_data.take(QStringLiteral("XDisplay")); | |
| auto &target = m_data[QStringLiteral("X11")]; | |
| for (auto it = legacy.cbegin(); it != legacy.cend(); ++it) { | |
| if (!target.contains(it.key())) { | |
| target.insert(it.key(), it.value()); | |
| } | |
| } | |
| } | |
| // Map "WaylandDisplay" -> "Wayland". | |
| if (m_data.contains(QStringLiteral("WaylandDisplay"))) { | |
| auto legacy = m_data.take(QStringLiteral("WaylandDisplay")); | |
| auto &target = m_data[QStringLiteral("Wayland")]; | |
| for (auto it = legacy.cbegin(); it != legacy.cend(); ++it) { | |
| if (!target.contains(it.key())) { | |
| target.insert(it.key(), it.value()); | |
| } | |
| } | |
| } |
| /** | ||
| * Get a configuration entry value. | ||
| * If the entry is not found in the loaded configuration, the default value is returned. | ||
| * If no default value is set, an exception is thrown. | ||
| * | ||
| * @param section The configuration section name. | ||
| * @param entry The configuration entry name. | ||
| * @return The value of the configuration entry. | ||
| */ | ||
| template<typename T> | ||
| inline T get(const QString §ion, const QString &entry) const { | ||
| if (m_data.contains(section) && m_data[section].contains(entry)) | ||
| return m_data[section][entry].value<T>(); | ||
| else if (m_defaults.contains(section) && m_defaults[section].contains(entry)) | ||
| return m_defaults[section][entry].first.value<T>(); | ||
| else | ||
| throw std::runtime_error(QStringLiteral("Config entry '%1/%2' not found") | ||
| .arg(section, entry) | ||
| .toStdString()); | ||
| } |
There was a problem hiding this comment.
The get() method throws std::runtime_error when a configuration entry is not found, but this exception type and the throwing behavior are not documented in the class documentation or the method's docstring. Callers may not expect exceptions and could fail to handle them properly, potentially causing crashes.
Add exception specifications to the documentation, or consider returning an optional/default value instead of throwing to make the API more robust and Qt-like. If exceptions are the intended design, document them clearly in both the class-level and method-level documentation.
get() 方法在找不到配置条目时抛出 std::runtime_error，但此异常类型和抛出行为未在类文档或方法的文档字符串中记录。调用者可能不期望异常，可能无法正确处理它们，可能导致崩溃。
建议在文档中添加异常规范，或考虑返回可选/默认值而不是抛出异常，以使 API 更健壮和更像 Qt。如果异常是预期的设计，请在类级别和方法级别的文档中明确记录它们。
| /** Main configuration instance. */ | ||
| inline Config mainConfig( | ||
| CONFIG_FILE, | ||
| { { {}, | ||
| { { "HaltCommand", { HALT_COMMAND, "Halt command" } }, | ||
| { "RebootCommand", { REBOOT_COMMAND, "Reboot command" } }, | ||
| { "Namespaces", | ||
| { QStringList(), | ||
| "List of Linux namespaces for user session to enter" } } } }, | ||
| { "Theme", | ||
| { { "CursorTheme", { QString(), "Cursor theme used in the greeter" } }, | ||
| { "CursorSize", { QString(), "Cursor size used in the greeter" } } } }, | ||
| { "X11", | ||
| { { "ServerPath", { "/usr/bin/X", "Path to X server binary" } }, | ||
| { "ServerArguments", | ||
| { "-nolisten tcp", "Arguments passed to the X server invocation" } }, | ||
| { "SessionDir", | ||
| { QStringList{ "/usr/local/share/xsessions", "/usr/share/xsessions" }, | ||
| "Comma-separated list of directories containing available X sessions" } }, | ||
| { "SessionCommand", | ||
| { SESSION_COMMAND, | ||
| "Path to a script to execute when starting the desktop session" } }, | ||
| { "SessionLogFile", | ||
| { ".local/share/ddm/xorg-session.log", "Path to the user session log file" } }, | ||
| { "DisplayCommand", | ||
| { QStringLiteral(DATA_INSTALL_DIR "/scripts/Xsetup"), | ||
| "Path to a script to execute when starting the display server" } }, | ||
| { "DisplayStopCommand", | ||
| { QStringLiteral(DATA_INSTALL_DIR "/scripts/Xstop"), | ||
| "Path to a script to execute when stopping the display server" } } } }, | ||
| { "Wayland", | ||
| { { "SessionDir", | ||
| { QStringList{ "/usr/local/share/wayland-sessions", "/usr/share/wayland-sessions" }, | ||
| "Comma-separated list of directories containing available Wayland sessions" } }, | ||
| { "SessionCommand", | ||
| { WAYLAND_SESSION_COMMAND, | ||
| "Path to a script to execute when starting the desktop session" } }, | ||
| { "SessionLogFile", | ||
| { ".local/share/ddm/wayland-session.log", | ||
| "Path to the user session log file" } } } }, | ||
| { "Single", | ||
| { { "SessionCommand", | ||
| { WAYLAND_SESSION_COMMAND, | ||
| "Path to a script to execute when starting the desktop session" } } } }, | ||
| { "Users", | ||
| { { "DefaultPath", | ||
| { "/usr/local/bin:/usr/bin:/bin", "Default $PATH for logged in users" } }, | ||
| { "RememberLastUser", { true, "Remember the last successfully logged in user" } }, | ||
| { "RememberLastSession", | ||
| { true, "Remember the session of the last successfully logged in user" } } } } }); | ||
|
|
||
| /** State configuration instance. */ | ||
| inline Config stateConfig( | ||
| QStringLiteral(STATE_DIR "/state.conf"), | ||
| { { "Last", | ||
| { { "User", { QString(), "The last successfully logged in user" } }, | ||
| { "Session", | ||
| { QString(), "The session of the last successfully logged in user" } } } } }); | ||
| } // namespace DDM |
There was a problem hiding this comment.
The inline global Config instances (mainConfig and stateConfig) are not thread-safe. These instances may be accessed from multiple threads (e.g., different Display instances, signal handlers, etc.), but the Config class has no synchronization mechanisms. Operations like load(), save(), get(), and set() can race with each other, potentially causing data corruption or crashes.
Since Config inherits from QObject, consider using QMutex to protect all member data access, or clearly document that these instances should only be accessed from the main thread.
内联全局 Config 实例（mainConfig 和 stateConfig）不是线程安全的。这些实例可能从多个线程访问（例如，不同的 Display 实例、信号处理程序等），但 Config 类没有同步机制。load()、save()、get() 和 set() 等操作可能相互竞争，可能导致数据损坏或崩溃。
由于 Config 继承自 QObject，建议使用 QMutex 保护所有成员数据访问，或明确记录这些实例只应从主线程访问。
| env.insert(QStringLiteral("DISPLAY"), display); | ||
| env.insert(QStringLiteral("HOME"), QStringLiteral("/")); | ||
| env.insert(QStringLiteral("PATH"), mainConfig.Users.DefaultPath.get()); | ||
| env.insert(QStringLiteral("PATH"), mainConfig.get<QString>("Users", "DefaultPath")); |
There was a problem hiding this comment.
Inconsistent indentation detected. This line uses spaces for indentation while surrounding lines use consistent formatting. The indentation should match the surrounding code style.
检测到不一致的缩进。此行使用空格缩进，而周围的行使用一致的格式。缩进应与周围的代码风格匹配。
| env.insert(QStringLiteral("PATH"), mainConfig.get<QString>("Users", "DefaultPath")); | |
| env.insert(QStringLiteral("PATH"), mainConfig.get<QString>("Users", "DefaultPath")); |
2 participants
The original ConfigReader is buggy and difficult for maintainence, refactor to make it both simple and keep backward compatibility.
Summary by Sourcery
Introduce a new generic configuration API and migrate existing components to use it while adjusting session management to use systemd login for TTY checks.
Enhancements: