feat: Upgrade treeland_ddm protocol to version 2#85
Open
calsys456 wants to merge 3 commits intolinuxdeepin:masterfrom
Open
feat: Upgrade treeland_ddm protocol to version 2#85calsys456 wants to merge 3 commits intolinuxdeepin:masterfrom
calsys456 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
|
Contributor
Author
|
Paired with linuxdeepin/treeland-protocols#47 |
![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 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.
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.
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);
| qWarning() << "Failed to find seat for VT" << vtnr; | ||
| return; | ||
| } | ||
| auto callback = display->connector->disableRender(); |
There was a problem hiding this comment.
disableRender() can return nullptr when proxy is not connected; calling wl_callback_add_listener() unconditionally will crash. Guard against null before adding the listener (and consider logging / directly acknowledging VT release when not connected).
当 proxy 未连接时，disableRender() 可能返回 nullptr；此处无条件调用 wl_callback_add_listener() 会崩溃。请在添加 listener 前判断是否为空（并考虑在未连接时记录日志/直接 ACK VT release）。
Suggested change
| auto callback = display->connector->disableRender(); | |
| auto callback = display->connector->disableRender(); | |
| if (!callback) { | |
| qWarning() << "Failed to disable render for VT" << vtnr << "- Treeland connector not connected?"; | |
| return; | |
| } |
Comment on lines
+344
to
+396
| void TreelandConnector::connect() { | ||
| if (m_connectTimer->isActive()) | ||
| return; | ||
| disconnect(); | ||
| tryConnect(); | ||
| } | ||
|
|
||
| void TreelandConnector::tryConnect() { | ||
| m_display = wl_display_connect("/run/treeland/wayland-0"); | ||
| if (!m_display) { | ||
| qInfo("Failed to connect to treeland, retrying..."); | ||
| if (!m_connectTimer->isActive()) | ||
| m_connectTimer->start(); | ||
| } else { | ||
| if (m_connectTimer->isActive()) | ||
| m_connectTimer->stop(); | ||
| connected(); | ||
| } | ||
| } | ||
|
|
||
| void TreelandConnector::connected() { | ||
| wl_registry *registry = wl_display_get_registry(m_display); | ||
| wl_registry_add_listener(registry, ®istryListener, this); | ||
| wl_display_roundtrip(m_display); | ||
|
|
||
| while (wl_display_dispatch_pending(m_display) > 0); | ||
| wl_display_flush(m_display); | ||
| m_notifier = new QSocketNotifier(wl_display_get_fd(m_display), QSocketNotifier::Read); | ||
| 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!"); | ||
| disconnect(); | ||
| // Auto reconnect | ||
| QTimer::singleShot(1000, this, &TreelandConnector::tryConnect); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| void TreelandConnector::disconnect() { | ||
| if (m_display) { | ||
| if (m_notifier) | ||
| m_notifier->setEnabled(false); | ||
| wl_display_disconnect(m_display); | ||
| if (m_notifier) { | ||
| m_notifier->deleteLater(); | ||
| m_notifier = nullptr; | ||
| } | ||
| m_display = nullptr; | ||
| } | ||
| }); | ||
| } | ||
| proxy = nullptr; | ||
| qInfo("Disconnected from treeland"); | ||
| } |
There was a problem hiding this comment.
disconnect() doesn’t stop m_connectTimer, and a previously scheduled QTimer::singleShot reconnection can still fire after an intentional disconnect (e.g. Display::stop()), causing unwanted background reconnect attempts. Consider stopping the timer in disconnect() and gating auto-reconnect behind a “shouldReconnect” flag or checking m_started/object state.
disconnect() 没有停止 m_connectTimer，并且之前通过 QTimer::singleShot 计划的重连在主动断开（例如 Display::stop()）后仍可能触发，导致后台继续重连。建议在 disconnect() 里停止 timer，并用“是否允许重连”的标志位控制自动重连（或检查 m_started/对象状态）。
Comment on lines
+71
to
+74
| QString idSeatStr = !idSeat || idSeat[0] == '\0' ? "seat0" : QString::fromLocal8Bit(idSeat); | ||
| auto res = std::ranges::find_if(daemonApp->seatManager()->displays, [&](const Display *d) { | ||
| return d->name == idSeatStr; | ||
| }); |
There was a problem hiding this comment.
std::ranges::find_if is used here but <algorithm> isn’t included in this translation unit. With -Werror this can fail to compile on some toolchains; add the proper standard header.
这里使用了 std::ranges::find_if，但该编译单元没有包含 <algorithm>。在某些工具链 + -Werror 下会导致编译失败，请补上对应的标准头文件。
Comment on lines
+308
to
+312
| if (strcmp(interface, "treeland_ddm_v2") == 0) { | ||
| auto conn = static_cast<TreelandConnector *>(data); | ||
| auto proxy = static_cast<struct treeland_ddm_v2 *>( | ||
| wl_registry_bind(registry, name, &treeland_ddm_v2_interface, version)); | ||
| treeland_ddm_v2_add_listener(proxy, &treelandDDMListener, conn); |
There was a problem hiding this comment.
wl_registry_bind(..., version) should cap version to the client-supported interface version (e.g. std::min(version, treeland_ddm_v2_interface.version)). Binding with the compositor’s higher advertised version can cause protocol mismatches.
wl_registry_bind(..., version) 应该把 version 限制到客户端支持的接口版本（例如 std::min(version, treeland_ddm_v2_interface.version)）。直接用合成器通告的更高版本可能导致协议不匹配。
Comment on lines
+153
to
+163
|
|
||
| // 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 guard = qScopeGuard([&] { | ||
| ioctl(fd, VT_RELDISP, VT_ACKACQ); | ||
| close(fd); | ||
| }); | ||
|
|
There was a problem hiding this comment.
open() can fail here (fd == -1). Calling VirtualTerminal::getVtActive(fd), ioctl(fd, ...), and close(fd) without checking can lead to undefined behavior and spurious VT switching failures. Handle the open() failure path explicitly.
这里 open() 可能失败（fd == -1）。不检查就调用 VirtualTerminal::getVtActive(fd)、ioctl(fd, ...) 和 close(fd) 可能导致未定义行为/VT 切换异常。请显式处理 open() 失败分支。
Suggested change
| // 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 guard = qScopeGuard([&] { | |
| ioctl(fd, VT_RELDISP, VT_ACKACQ); | |
| close(fd); | |
| }); | |
| if (fd < 0) { | |
| qWarning() << "Failed to open VT device for acquire:" << defaultVtPath << "errno" << errno; | |
| return; | |
| } | |
| // 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. | |
| auto guard = qScopeGuard([&] { | |
| ioctl(fd, VT_RELDISP, VT_ACKACQ); | |
| close(fd); | |
| }); | |
| int vtnr = VirtualTerminal::getVtActive(fd); |
Comment on lines
279
to
285
| // Retrieve XDG_SESSION_ID | ||
| xdgSessionId = env.value(QStringLiteral("XDG_SESSION_ID")).toInt(); | ||
| if (xdgSessionId <= 0) { | ||
| qCritical() << "[SessionLeader] Invalid XDG_SESSION_ID from pam_open_session()"; | ||
| exit(1); | ||
| } | ||
| if (write(pipefd[1], &xdgSessionId, sizeof(int)) != sizeof(int)) { | ||
| session = env.value(QStringLiteral("XDG_SESSION_ID")); | ||
| QByteArray sessionBa = session.toLocal8Bit(); | ||
| strcpy(xdgSessionId, sessionBa.constData()); | ||
| if (write(pipefd[1], &xdgSessionId, sizeof(char) * 128) != sizeof(char) * 128) { | ||
| qCritical() << "[SessionLeader] Failed to write XDG_SESSION_ID to parent process!"; | ||
| exit(1); |
There was a problem hiding this comment.
strcpy(xdgSessionId, sessionBa.constData()) can overflow the fixed 128-byte buffer if the session ID is longer than expected, leading to memory corruption. Use a bounded copy (e.g. qstrncpy/strncpy) and also treat an empty XDG_SESSION_ID as a hard error (exit) to avoid starting a session that the parent will consider failed.
strcpy(xdgSessionId, sessionBa.constData()) 在会话 ID 超出 128 字节时会溢出固定缓冲区，造成内存破坏。请使用有界拷贝（如 qstrncpy/strncpy），并且当 XDG_SESSION_ID 为空时应直接视为致命错误并退出，避免子进程继续启动而父进程判定失败。
Comment on lines
290
to
313
| @@ -361,8 +312,8 @@ namespace DDM { | |||
| qInfo() << "Successfully logged in user" << user; | |||
| } | |||
There was a problem hiding this comment.
After a successful openSession(), the daemon doesn’t notify Treeland of the new session via connector->userLoggedIn(user, auth->session). Without this, Treeland may not learn the session ID needed for later lock/logout requests (only existing sessions are reported during initial global bind). Add the notification after the session is opened.
openSession() 成功后，daemon 没有通过 connector->userLoggedIn(user, auth->session) 通知 Treeland 新会话信息。这样 Treeland 可能拿不到后续 lock/logout 需要的 session ID（目前只在初次 bind global 时回放已存在会话）。请在会话打开成功后补上该通知。
Comment on lines
+62
to
+70
| if (idSeat) | ||
| free((void *)idSeat); | ||
| }); | ||
| sd_device_new_from_path(&device, qPrintable(path)); | ||
| if (!device) { | ||
| qWarning() << "Failed to get seat for device" << path; | ||
| return nullptr; | ||
| } | ||
| sd_device_get_property_value(device, "ID_SEAT", &idSeat); |
There was a problem hiding this comment.
sd_device_get_property_value() returns a pointer owned by sd_device (valid until unref); it must not be free()d. The current scope guard freeing idSeat can cause invalid free/crash. Remove the free((void*)idSeat) and (optionally) check return codes from the sd-device calls.
sd_device_get_property_value() 返回的字符串指针由 sd_device 持有（直到 unref 前都有效），不应该被 free()。当前 scope guard 里释放 idSeat 可能导致非法释放/崩溃。请移除 free((void*)idSeat)，并（可选）检查 sd-device 调用的返回值。
Suggested change
| if (idSeat) | |
| free((void *)idSeat); | |
| }); | |
| sd_device_new_from_path(&device, qPrintable(path)); | |
| if (!device) { | |
| qWarning() << "Failed to get seat for device" << path; | |
| return nullptr; | |
| } | |
| sd_device_get_property_value(device, "ID_SEAT", &idSeat); | |
| }); | |
| int r = sd_device_new_from_path(&device, qPrintable(path)); | |
| if (r < 0 || !device) { | |
| qWarning() << "Failed to get seat for device" << path << "error:" << strerror(-r); | |
| return nullptr; | |
| } | |
| r = sd_device_get_property_value(device, "ID_SEAT", &idSeat); | |
| if (r < 0 && r != -ENOENT) { | |
| qWarning() << "Failed to get ID_SEAT for device" << path << "error:" << strerror(-r); | |
| } |
| #include <QObject> | ||
| #include <QScopeGuard> | ||
| #include <QSocketDescriptor> | ||
| #include <QSocketNotifier> |
There was a problem hiding this comment.
This file uses QTimer but does not include <QTimer>, which can break the build depending on transitive includes. Add the missing include (and prefer explicit includes for Qt types used).
该文件使用了 QTimer 但没有 #include <QTimer>，在缺少传递包含时可能导致编译失败。请补上缺失的 include（并尽量显式包含所用到的 Qt 类型头文件）。
Suggested change
| #include <QSocketNotifier> | |
| #include <QSocketNotifier> | |
| #include <QTimer> |
|
TAG Bot New tag: 0.3.3 |
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.
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: