feat: Upgrade treeland_ddm protocol to version 2#85
feat: Upgrade treeland_ddm protocol to version 2#85calsys456 wants to merge 3 commits intolinuxdeepin:masterfrom
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 Treeland/DDM integration to use the new Wayland-based treeland_ddm_v2 protocol instead of the legacy v1 socket-based approach, adds per-seat TreelandConnector instances and richer Wayland event handlers, updates authentication/session handling to use string XDG session IDs, and removes the custom SocketServer IPC layer from the daemon. Sequence diagram for treeland_ddm_v2 login flow and authentication resultsequenceDiagram
participant TClient as Treeland_client
participant WL as treeland_ddm_v2
participant TC as TreelandConnector
participant D as Display
participant A as Auth
participant PAM as PAM_Logind
TClient->>WL: login(username, secret, session_type, session_file)
WL->>TC: login(username, secret, session_type, session_file)
TC->>D: login(username, secret, Session)
alt invalid user dde
D-->>TC: authenticationFailed(INVALID_USER)
TC->>WL: authentication_failed(error=INVALID_USER)
else existing authentication ongoing
D-->>TC: authenticationFailed(EXISTING_AUTHENTICATION_ONGOING)
TC->>WL: authentication_failed(error=EXISTING_AUTHENTICATION_ONGOING)
else invalid session metadata
D-->>TC: authenticationFailed(INVALID_SESSION)
TC->>WL: authentication_failed(error=INVALID_SESSION)
else credentials and session valid
D->>A: new Auth(user)
D->>A: authenticate(secret)
alt authentication failed
A-->>D: false
D-->>TC: authenticationFailed(AUTHENTICATION_FAILED)
TC->>WL: authentication_failed(error=AUTHENTICATION_FAILED)
else authentication succeeded
A-->>D: true
D->>A: openSession(exec, env, cookie)
A->>PAM: pam_open_session()
A->>PAM: start user session process
PAM-->>A: XDG_SESSION_ID (string), session PID
A-->>D: session id string
D->>PAM: register session via logind
D-->>TC: userLoggedIn(username, session)
TC->>WL: user_logged_in(username, session)
end
end
Class diagram for updated TreelandConnector, Display, Auth, TreelandDisplayServerclassDiagram
direction LR
class DaemonApp {
+DisplayManager* displayManager()
+PowerManager* powerManager()
+SeatManager* seatManager()
+SignalHandler* signalHandler()
}
class SeatManager {
+QList~Display*~ displays
+void switchToGreeter(QString name)
}
class Display {
+QString name
+int terminalId
+QList~Auth*~ auths
+TreelandConnector* connector
+TreelandDisplayServer* m_treeland
+XorgDisplayServer* m_x11Server
+bool start()
+void stop()
+void login(QString user, QString password, Session session)
+void logout(QString session)
+void lock(QString session)
+void unlock(QString user, QString password)
<<signal>> void stopped()
}
class TreelandConnector {
+TreelandConnector(Display* display)
+~TreelandConnector()
+void connect()
+void disconnect()
+void capabilities(uint32_t capabilities) const
+void userLoggedIn(QString username, QString session) const
+void authenticationFailed(uint32_t error) const
+void switchToGreeter() const
+void switchToUser(QString username) const
+void activateSession() const
+void deactivateSession() const
+void enableRender() const
+wl_callback* disableRender() const
-- Wayland state --
+treeland_ddm_v2* proxy
-wl_display* m_display
-QSocketNotifier* m_notifier
-QTimer* m_connectTimer
-- slots --
-void tryConnect()
-void connected()
}
class TreelandDisplayServer {
+TreelandDisplayServer(Display* parent)
+~TreelandDisplayServer()
+bool start()
+void stop()
-bool m_started
}
class Auth {
+QString user
+int tty
+QString session
+pid_t sessionLeaderPid
+bool sessionOpened
+bool authenticated
+Auth(QObject* parent, QString user)
+bool authenticate(QByteArray password)
+QString openSession(QString command, QProcessEnvironment env, QByteArray cookie)
+void closeSession()
}
class PowerManager {
+uint32_t capabilities() const
+void powerOff()
+void reboot()
+void suspend()
+void hibernate()
+void hybridSleep()
}
class VirtualTerminal {
+static QString path(int vtnr)
+static void setVtSignalHandler(void(*onAcquireDisplay)(), void(*onReleaseDisplay)())
+static int getVtActive(int fd)
+static int currentVt()
+static void handleVtSwitches(int fd)
+static void jumpToVt(int tty, bool wait, bool autoSwitchBack)
}
class treeland_ddm_v2 {
<<Wayland interface>>
}
%% Relationships
DaemonApp --> SeatManager
DaemonApp --> PowerManager
SeatManager "1" o-- "*" Display
Display "1" o-- "1" TreelandDisplayServer : owns
Display "1" o-- "1" TreelandConnector : owns
Display "1" o-- "*" Auth : manages
TreelandConnector --> Display : parent
TreelandConnector --> treeland_ddm_v2 : proxy
TreelandConnector --> PowerManager : uses
TreelandConnector --> VirtualTerminal : uses
TreelandDisplayServer --> Display : parent
Auth --> VirtualTerminal : uses
SeatManager --> TreelandConnector : uses switchToGreeter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Paired with linuxdeepin/treeland-protocols#47 |
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
findSeatOfDevice, theidSeatpointer returned fromsd_device_get_property_valueis owned bysd_deviceand must not be freed; dropping the manualfree(idSeat)in the scope guard will avoid undefined behavior and potential crashes. - In
onAcquireDisplay, the file descriptor opened fordefaultVtPathis leaked whenfindSeatOfDevicefails (earlyreturnwithoutclose(fd)); ensure the fd is always closed, e.g. via a scope guard. - In
Auth::openSession, the fixed-sizechar xdgSessionId[128]buffer andstrcpyfrom aQByteArraycan overflow ifXDG_SESSION_IDexceeds the buffer size; consider usingqstrncpyor checking the length before copying.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `findSeatOfDevice`, the `idSeat` pointer returned from `sd_device_get_property_value` is owned by `sd_device` and must not be freed; dropping the manual `free(idSeat)` in the scope guard will avoid undefined behavior and potential crashes.
- In `onAcquireDisplay`, the file descriptor opened for `defaultVtPath` is leaked when `findSeatOfDevice` fails (early `return` without `close(fd)`); ensure the fd is always closed, e.g. via a scope guard.
- In `Auth::openSession`, the fixed-size `char xdgSessionId[128]` buffer and `strcpy` from a `QByteArray` can overflow if `XDG_SESSION_ID` exceeds the buffer size; consider using `qstrncpy` or checking the length before copying.
## Individual Comments
### Comment 1
<location path="src/daemon/TreelandConnector.cpp" line_range="104-113" />
<code_context>
+ .done = renderDisabled,
+ };
+
+ static void onAcquireDisplay() {
+ int fd = open(defaultVtPath, O_RDWR | O_NOCTTY);
+
+ // Activate treeland session before we acknowledge VT switch.
+ // This will queue our rendering jobs before any keyboard event, to ensure
+ // all GUI elements are under a prepared state before next possible VT
+ // switch issued by keyboard.
+ int vtnr = VirtualTerminal::getVtActive(fd);
+ auto display = findSeatOfDevice(VirtualTerminal::path(vtnr));
+ if (!display) {
+ qWarning() << "Failed to find seat for VT" << vtnr;
+ return;
+ }
+ auto conn = display->connector;
+ auto user = daemonApp->displayManager()->findUserByVt(vtnr);
+ if (isVtRunningTreeland(vtnr)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** File descriptor leak and missing VT_ACKACQ on early return in onAcquireDisplay.
In `onAcquireDisplay()`, if `findSeatOfDevice` fails we log and return without closing `fd` or issuing `VT_ACKACQ`. This leaks the VT file descriptor and leaves the kernel waiting for an ack, which can break VT switching. Please ensure the ioctl and close happen on all paths (e.g. via a common cleanup / RAII-style scope guard) or explicitly handle the failure case appropriately.
</issue_to_address>
### Comment 2
<location path="src/daemon/Auth.cpp" line_range="232-236" />
<code_context>
+ return {};
}
+ char xdgSessionId[128] = {};
+
// Here is most safe place to jump VT
</code_context>
<issue_to_address>
**🚨 issue (security):** Potential buffer overflow and brittle fixed-size protocol for XDG_SESSION_ID exchange.
`xdgSessionId` is a fixed 128-byte buffer and is filled via `strcpy` while both sides assume a fixed 128-byte read/write. If `XDG_SESSION_ID` exceeds 127 bytes this will overflow the buffer. Please either bound the copy and/or send a length-prefixed string so only the actual length is transmitted. Additionally, the parent should verify that the full expected number of bytes is read, not just check `read(...) < 0`.
</issue_to_address>
### Comment 3
<location path="src/daemon/TreelandConnector.cpp" line_range="71-80" />
<code_context>
+ } else {
+ auto active = tty.readAll();
+ tty.close();
+ int scanResult = sscanf(qPrintable(active), "tty%d", &activeVt);
+ if (scanResult != 1) {
+ qWarning(
+ "Failed to parse active VT from /sys/class/tty/tty0/active with content %s",
+ qPrintable(active));
+ return;
}
}
</code_context>
<issue_to_address>
**question (bug_risk):** New early return on parse failure changes behavior and might leave session state unsynchronized.
Previously, a parse failure set `activeVt` to -1 and the rest of the logic still ran (e.g. `findUserByVt(-1)`), so session/Treeland handling still executed. With the new early return on `scanResult != 1`, a transient parse error will now skip all that logic after acknowledging VT release, so the connector may never switch to the next VT or deactivate the session. If that change in behavior isn’t intentional, consider either preserving the sentinel `activeVt` flow or adding an explicit error-handling path that still maintains session state correctly.
</issue_to_address>
### Comment 4
<location path="src/daemon/TreelandConnector.cpp" line_range="55" />
<code_context>
- qWarning("Failed to parse active VT from /sys/class/tty/tty0/active with content %s", qPrintable(active));
- activeVt = -1;
+
+ static Display *findSeatOfDevice(QString path) {
+ // Get the seat of the active VT
+ sd_device *device = nullptr;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting shared helpers for VT-to-display lookup, FD management, notifier teardown, and display access to reduce duplication and clarify the TreelandConnector control flow.
A few targeted refactors would reduce complexity and duplication without changing behavior.
### 1. Centralize VT → Display mapping
`renderDisabled`, `onAcquireDisplay`, and `onReleaseDisplay` all do:
- derive VT number
- call `findSeatOfDevice(VirtualTerminal::path(vtnr))`
- then use `display->connector`
That mapping can be made explicit and reused:
```cpp
// in anonymous namespace
static Display *displayForVt(int vtnr)
{
return findSeatOfDevice(VirtualTerminal::path(vtnr));
}
// usage:
static void onAcquireDisplay()
{
int fd = open(defaultVtPath, O_RDWR | O_NOCTTY);
if (fd < 0) {
qWarning("Failed to open VT: %s", strerror(errno));
return;
}
auto fdGuard = qScopeGuard([&] { close(fd); });
int vtnr = VirtualTerminal::getVtActive(fd);
auto display = displayForVt(vtnr);
if (!display) {
qWarning() << "Failed to find seat for VT" << vtnr;
return;
}
auto conn = display->connector;
auto user = daemonApp->displayManager()->findUserByVt(vtnr);
if (isVtRunningTreeland(vtnr)) {
qDebug("Activate session at VT %d for user %s", vtnr, qPrintable(user));
conn->activateSession();
conn->enableRender();
conn->switchToUser(user);
}
ioctl(fd, VT_RELDISP, VT_ACKACQ);
}
```
That removes repeated `VirtualTerminal::path(...)` + `findSeatOfDevice(...)` logic and makes the intent (“display for VT”) explicit. You can apply the same helper in `renderDisabled` and `onReleaseDisplay`.
### 2. Use RAII/QScopeGuard consistently for FDs
You already use `QScopeGuard` for `sd_device`. Applying it to VT FDs simplifies control flow and fixes early-return leaks (e.g. `onAcquireDisplay` returning before `close(fd)`):
```cpp
static void renderDisabled(void * /*data*/, struct wl_callback *callback, uint32_t /*serial*/)
{
wl_callback_destroy(callback);
int fd = open(defaultVtPath, O_RDWR | O_NOCTTY);
if (fd < 0) {
qWarning("Failed to open VT for VT_RELDISP: %s", strerror(errno));
return;
}
auto fdGuard = qScopeGuard([&] { close(fd); });
ioctl(fd, VT_RELDISP, 1);
// ... rest unchanged, no manual close(fd) needed
}
```
Same pattern can be applied to:
- `onAcquireDisplay`
- `switchToVt`
- VT handling inside `registerGlobal` (the `handleVtSwitches` call)
This removes the need to reason about each early return and keeps FDs safe by construction.
### 3. Avoid duplicated teardown in the socket notifier lambda
`connected()`’s notifier lambda reimplements teardown that already exists in `disconnect()`:
```cpp
QObject::connect(m_notifier, &QSocketNotifier::activated, this, [&] {
if ((wl_display_dispatch(m_display) == -1 || wl_display_flush(m_display) == -1)
&& errno != EAGAIN) {
qWarning("Treeland connection lost!");
m_notifier->setEnabled(false);
m_notifier->deleteLater();
m_notifier = nullptr;
wl_display_disconnect(m_display);
m_display = nullptr;
proxy = nullptr;
QTimer::singleShot(1000, this, &TreelandConnector::tryConnect);
}
});
```
You can delegate teardown to `disconnect()` and keep reconnect logic local:
```cpp
QObject::connect(m_notifier, &QSocketNotifier::activated, this, [this] {
if ((wl_display_dispatch(m_display) == -1 || wl_display_flush(m_display) == -1)
&& errno != EAGAIN) {
qWarning("Treeland connection lost!");
disconnect(); // single teardown path
QTimer::singleShot(1000, this, &TreelandConnector::tryConnect);
}
});
```
This keeps all connection shutdown logic in one place and shrinks the lambda significantly.
### 4. Remove repeated `static_cast<Display*>` boilerplate
Most event handlers start with:
```cpp
auto connector = static_cast<TreelandConnector *>(data);
auto display = static_cast<Display *>(connector->parent());
```
You can either:
**(a) Add a tiny helper:**
```cpp
static Display *displayFromData(void *data)
{
auto connector = static_cast<TreelandConnector *>(data);
return static_cast<Display *>(connector->parent());
}
// usage
static void login(void *data, treeland_ddm_v2 * /*ddm*/, const char *username,
const char *secret, uint32_t session_type, const char *session_file)
{
auto display = displayFromData(data);
display->login(...);
}
```
or
**(b) Store the `Display*` in `TreelandConnector`:**
```cpp
// TreelandConnector.h
class TreelandConnector : public QObject {
public:
explicit TreelandConnector(Display *display);
Display *display() const { return m_display; }
private:
Display *m_display = nullptr;
};
// TreelandConnector.cpp
TreelandConnector::TreelandConnector(Display *display)
: QObject(display)
, m_display(display)
{
// ...
}
// handler
static void login(void *data, treeland_ddm_v2 * /*ddm*/, const char *username,
const char *secret, uint32_t session_type, const char *session_file)
{
auto conn = static_cast<TreelandConnector *>(data);
auto display = conn->display();
display->login(...);
}
```
Either option makes the ownership assumption explicit and removes repeated casts from all handlers (`login/logout/lock/unlock`, etc.), improving readability.
</issue_to_address>
### Comment 5
<location path="src/daemon/TreelandConnector.h" line_range="34" />
<code_context>
+ void enableRender() const;
+ struct wl_callback *disableRender() const;
+
+ struct treeland_ddm_v2 *proxy{ nullptr };
+
+ private Q_SLOTS:
</code_context>
<issue_to_address>
**issue (complexity):** Consider hiding the raw protocol pointer behind a private member and framing `connect()` as a simple high-level entry point to reduce the visible API surface and lifecycle complexity for callers.
You can keep the new functionality as-is while tightening encapsulation and simplifying the public surface a bit.
### 1. Hide `treeland_ddm_v2 *proxy` behind a private member
Exposing the raw protocol pointer directly makes the type part of your public API and encourages external code to poke at it. You can keep the same behavior but hide the pointer and expose a minimal accessor for internal/legacy uses.
```cpp
namespace DDM {
class TreelandConnector : public QObject
{
Q_OBJECT
public:
TreelandConnector(Display *display);
~TreelandConnector();
void connect();
void disconnect();
void capabilities(uint32_t capabilities) const;
void userLoggedIn(const QString &username, const QString &session) const;
void authenticationFailed(uint32_t error) const;
void switchToGreeter() const;
void switchToUser(const QString &username) const;
void activateSession() const;
void deactivateSession() const;
void enableRender() const;
struct wl_callback *disableRender() const;
// If absolutely needed, expose a narrow accessor instead of a public field
struct treeland_ddm_v2 *proxy() const { return m_proxy; }
private Q_SLOTS:
void tryConnect();
void connected();
private:
struct wl_display *m_display{ nullptr };
QSocketNotifier *m_notifier{ nullptr };
QTimer *m_connectTimer{ nullptr };
struct treeland_ddm_v2 *m_proxy{ nullptr };
};
} // namespace DDM
```
In the `.cpp` file, update all uses of `proxy` to `m_proxy` and, if external code used `connector.proxy`, switch it to `connector.proxy()`.
### 2. Clarify connection lifecycle API
Right now the header exposes `connect()` and private slots `tryConnect()` / `connected()`. Callers only need a simple “ensure there’s an active connection” entry point; the rest is internal state-machine logic.
You can keep the reconnection behavior but make the intent clearer:
```cpp
class TreelandConnector : public QObject
{
Q_OBJECT
public:
// High-level API: caller just asks for a connection
void connect(); // or rename to ensureConnected()
private Q_SLOTS:
// Internal state machine
void tryConnect();
void connected();
private:
bool m_isConnected{false};
};
```
And in the implementation:
```cpp
void TreelandConnector::connect()
{
if (m_isConnected) {
return;
}
// Kick off internal state machine
tryConnect();
}
void TreelandConnector::connected()
{
m_isConnected = true;
// existing logic...
}
```
This keeps all of your reconnection logic intact but reduces the conceptual surface for users of `TreelandConnector` to “call `connect()` once, we take care of the rest.”
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR upgrades ddm’s Treeland integration from the legacy local-socket interface to the treeland_ddm_v2 Wayland protocol, moving greeter/daemon interactions into per-display Wayland connections.
Changes:
- Remove
SocketServer-based greeter IPC and related Treeland socket tracking. - Introduce per-
DisplayTreelandConnectorinstances that bindtreeland_ddm_v2, implement protocol event handling, and send capability/session updates. - Switch logind session identifiers from integer IDs to opaque string
XDG_SESSION_IDvalues, updating login/lock/unlock/logout flows accordingly.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/daemon/TreelandDisplayServer.h |
Drops socket-server dependencies and greeter socket state from the display server wrapper. |
src/daemon/TreelandDisplayServer.cpp |
Removes user activation/login-failed socket messaging; keeps start/stop lifecycle only. |
src/daemon/TreelandConnector.h |
Refactors connector API to treeland_ddm_v2, adds reconnect scaffolding and request wrappers. |
src/daemon/TreelandConnector.cpp |
Implements v2 event handlers (login/logout/lock/unlock/power/VT) + per-display VT routing and reconnect loop. |
src/daemon/SocketServer.h |
Removed legacy local-socket server interface. |
src/daemon/SocketServer.cpp |
Removed legacy local-socket protocol implementation. |
src/daemon/SeatManager.cpp |
Routes “switch to greeter” via per-display connector->switchToGreeter(). |
src/daemon/Display.h |
Removes socket-based slots/signals; adds per-display TreelandConnector*. |
src/daemon/Display.cpp |
Creates/uses per-display connector; adapts login/lock/unlock/logout and error reporting to v2. |
src/daemon/DaemonApp.h |
Removes daemon-global TreelandConnector accessor/member. |
src/daemon/DaemonApp.cpp |
Stops constructing a daemon-global connector. |
src/daemon/CMakeLists.txt |
Regenerates Wayland client code for treeland-ddm-v2.xml and removes SocketServer.cpp from sources. |
src/daemon/Auth.h |
Changes logind session ID storage/return type to QString. |
src/daemon/Auth.cpp |
Propagates string XDG_SESSION_ID from child to parent via pipe and returns it. |
REUSE.toml |
Updates REUSE annotations to remove deleted SocketServer paths. |
(中文翻译)
本 PR 将 ddm 与 Treeland 的集成从本地 socket 旧接口升级到 treeland_ddm_v2 Wayland 协议,把 greeter/daemon 的交互迁移到按 Display 维度的 Wayland 通信中。
变更点:
- 移除基于
SocketServer的 greeter IPC 以及相关 socket 跟踪逻辑。 - 为每个
Display引入独立的TreelandConnector,绑定treeland_ddm_v2,处理协议事件并上报能力/会话信息。 - 将 logind 会话标识从整型改为不透明字符串
XDG_SESSION_ID,并同步调整登录/锁定/解锁/注销流程。
Comments suppressed due to low confidence (1)
src/daemon/Display.h:97
- The slot section comment and parameter name are now misleading: these slots are no longer “socket” based, and
unlocktakes a username (seeTreelandConnector::unlockevent) rather than a session. Please update the comment and rename the parameter to match its meaning.
这里的注释和参数名已产生误导:这些 slot 已不再基于“socket”,并且 unlock 的第一个参数实际是用户名(见 TreelandConnector::unlock 事件),而不是 session。请更新注释并把参数名改为符合语义的命名。
///////////////////////////////////////////////////
// Slots for socket to communicate with Treeland //
///////////////////////////////////////////////////
void login(const QString &user,
const QString &password,
const Session &session);
void logout(const QString &session);
void lock(const QString &session);
void unlock(const QString &session, const QString &password);
|
TAG Bot New tag: 0.3.3 |
In this upgrade we rewrite all treeland-ddm interactions, includes those originally in SocketServer / GreeterProxy, into wayland communication. There's only one true treeland-ddm interaction which should on behalf of the Wayland protocol.
With the upgrade we did in previous commit, SocketServer is not needed now, and we shall reform our communication with our greeter the Treeland, to use the newly formed Wayland protocol.
Although the logind session id is exactly an integer in current systemd-logind, but this is an UB according to systemd documentation. Systemd advertise us to treat it as a string and we shall adapt it.
In this upgrade we rewrite all treeland-ddm interactions, includes those originally in SocketServer / GreeterProxy, into wayland communication. There's only one true treeland-ddm interaction which should on behalf of the Wayland protocol.
Summary by Sourcery
Upgrade the Treeland daemon’s integration to the treeland_ddm v2 Wayland protocol and remove the legacy local-socket-based interface.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation: