chore: replace org.freedesktop.login1 D-Bus call with sd-login API #78
Conversation
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
|
[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 TTY-in-use detection to use systemd sd-login APIs instead of org.freedesktop.login1 D-Bus interfaces, along with minor include and header ordering cleanups in Display.cpp. Sequence diagram for TTY in-use check via systemd sd-login APIssequenceDiagram
participant Display as DDM_Display_isTtyInUse
participant SdLogin as sd_login_library
participant Logind as systemd_logind_service
Display->>SdLogin: sd_get_sessions(sessions)
SdLogin->>Logind: query_active_sessions
Logind-->>SdLogin: list_of_sessions
loop for each session in sessions
Display->>SdLogin: sd_session_get_tty(session, tty)
SdLogin->>Logind: get_tty_for_session
Logind-->>SdLogin: tty
Display->>SdLogin: sd_session_get_state(session, state)
SdLogin->>Logind: get_state_for_session
Logind-->>SdLogin: state
alt tty equals desiredTty and state is not closing
Display-->>Display: return true
end
end
Display-->>Display: return false
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 2 issues, and left some high level feedback:
- The RAII guards in
isTtyInUsecapturesessions,tty, andstateby value at construction time (when they are stillnullptr), so the deleters will never free the actual buffers returned bysd_get_sessions/sd_session_get_*; instead, manage the lifetime by wrapping the actual pointer variables (e.g., via a small helper struct or astd::unique_ptr<T*, Deleter>that owns and mutates the same pointer). - The return values of
sd_get_sessions,sd_session_get_tty, andsd_session_get_stateare not checked before use; these calls should be validated andtty/statetreated as potentially null before comparing withdesiredTtyor passing tostrcmpto avoid undefined behavior on error paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The RAII guards in `isTtyInUse` capture `sessions`, `tty`, and `state` by value at construction time (when they are still `nullptr`), so the deleters will never free the actual buffers returned by `sd_get_sessions`/`sd_session_get_*`; instead, manage the lifetime by wrapping the actual pointer variables (e.g., via a small helper struct or a `std::unique_ptr<T*, Deleter>` that owns and mutates the same pointer).
- The return values of `sd_get_sessions`, `sd_session_get_tty`, and `sd_session_get_state` are not checked before use; these calls should be validated and `tty`/`state` treated as potentially null before comparing with `desiredTty` or passing to `strcmp` to avoid undefined behavior on error paths.
## Individual Comments
### Comment 1
<location> `src/daemon/Display.cpp:61-64` </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)
</code_context>
<issue_to_address>
**issue (bug_risk):** RAII guards capture `sessions`/`tty`/`state` by value, so they never see the updated pointers and won't free anything.
Because `guard` and `guard2` capture `sessions`, `tty`, and `state` by value, they store the initial `nullptr` values. When you later assign to those variables, the captured copies in the deleter are not updated, so nothing gets freed and the allocations from `sd_get_sessions` / `sd_session_get_tty` / `sd_session_get_state` are leaked. Capture by reference instead (e.g. `[&sessions]`, `[&tty, &state]`), or encapsulate the raw pointers in a small RAII struct and use that as the `unique_ptr` deleter target instead of capturing variables in a `std::function`.
</issue_to_address>
### Comment 2
<location> `src/daemon/Display.cpp:83-85` </location>
<code_context>
+ 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):** Return codes and nullability from sd-login APIs are not checked, which can lead to UB via `strcmp` on a null pointer.
`sd_get_sessions`, `sd_session_get_tty`, and `sd_session_get_state` return values are ignored, so `tty`/`state` may be null. That makes `strcmp(state, "closing")` undefined if `state` is null, and comparing `desiredTty` to a null `tty` is almost certainly unintended. Please check each `sd_*` return value, handle errors by skipping/handling the session, and guard against `state == nullptr` (and invalid `tty`) before use.
</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 replaces the login1 D-Bus–based session/TTY lookup with systemd sd-login APIs to determine whether a VT/TTY is already in use, aiming to make VT selection more robust.
Changes:
- Reworked
isTtyInUse()to iterate sessions viasd_get_sessions()and inspectsd_session_get_tty()/sd_session_get_state(). - Updated includes accordingly (adds
systemd/sd-login.h, reshuffles related headers).
This is more robustic.
Comment on lines
+80
to
+82
| if (sd_session_get_tty(*s, &tty) < 0 || sd_session_get_state(*s, &state) < 0) | ||
| continue; | ||
| if (desiredTty == tty && strcmp(state, "closing") != 0) { |
There was a problem hiding this comment.
This function uses strcmp()/free() but doesn’t include <cstring> / <cstdlib> (or <string.h> / <stdlib.h>), so it relies on transitive includes. Please add the explicit standard headers (and consider std::strcmp/std::free) to avoid build breaks when upstream headers change.
这里使用了 strcmp()/free()，但没有显式包含 <cstring> / <cstdlib>（或 <string.h> / <stdlib.h>），目前依赖间接包含。建议补齐标准头文件（并考虑使用 std::strcmp/std::free），避免上游头文件变更导致编译失败。
Comment on lines
+41
to
+42
| #include <QDBusMessage> | ||
| #include <QDBusReply> |
There was a problem hiding this comment.
#include <QDBusMessage> and #include <QDBusReply> appear unused in this file (no references outside the includes). Please remove them to keep dependencies minimal.
#include <QDBusMessage> 和 #include <QDBusReply> 在此文件中看起来未被使用（除 include 外没有引用）。建议移除以保持依赖最小化。
Suggested change
| #include <QDBusMessage> | |
| #include <QDBusReply> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This is more robustic.
Summary by Sourcery
Enhancements: