-
Notifications
You must be signed in to change notification settings - Fork 16
fix: Fix dde-session not start after treeland logged in #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This will work as intended, at least for now...
|
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 guide (collapsed on small PRs)Reviewer's GuideAdjusts Treeland VT signal handler setup to temporarily disable it during session process startup to avoid a VT RELDISP-induced malloc deadlock, and encapsulates the handler registration in TreelandConnector so it can be reapplied after login. Sequence diagram for VT signal handler handling during session startupsequenceDiagram
participant Display
participant Auth
participant VirtualTerminal
participant DaemonApp
participant TreelandConnector
Display->>Auth: connect(userProcessFinished)
Display->>VirtualTerminal: setVtSignalHandler(nullptr, nullptr)
Display->>Auth: startUserProcess(sessionExec, cookie)
Auth-->>Display: userProcessStarted
Display->>DaemonApp: treelandConnector()
DaemonApp-->>Display: TreelandConnector
Display->>TreelandConnector: setSignalHandler()
TreelandConnector->>VirtualTerminal: setVtSignalHandler(onAcquireDisplay, onReleaseDisplay)
Auth-->>Display: userProcessFinished(exitCode)
Updated class diagram for TreelandConnector VT signal handler managementclassDiagram
class Display {
+userProcessFinished()
}
class Auth {
+startUserProcess(execPath, cookie)
}
class VirtualTerminal {
+setVtSignalHandler(onAcquireCallback, onReleaseCallback)
}
class TreelandConnector {
+TreelandConnector()
+~TreelandConnector()
+isConnected() bool
+setPrivateObject(ddm)
+setSignalHandler()
+connect(socketPath)
+switchToGreeter()
}
class DaemonApp {
+treelandConnector() TreelandConnector*
}
Display --> Auth : uses
Display --> DaemonApp : uses
DaemonApp --> TreelandConnector : provides
TreelandConnector --> VirtualTerminal : configures VT signals
Display --> VirtualTerminal : temporarily disables VT signals
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- The FIXME comment about VT RELDISP and glibc malloc deadlock is the only explanation for the change; consider briefly documenting why moving
setVtSignalHandlerafterstartUserProcessavoids the deadlock and what assumptions this relies on, so future refactors don’t inadvertently regress it. - Now that
TreelandConnector’s constructor no longer unconditionally sets the VT signal handler, make it explicit in the class contract (e.g., via naming or comments) thatsetSignalHandler()must be called after construction, and consider guarding against multiple calls or ensuring it’s safe to call idempotently. - Review whether the destructor should explicitly reset or clear the VT signal handler now that it is set later via
setSignalHandler()rather than in the constructor, to avoid leaving global state in an unexpected configuration on teardown.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The FIXME comment about VT RELDISP and glibc malloc deadlock is the only explanation for the change; consider briefly documenting why moving `setVtSignalHandler` after `startUserProcess` avoids the deadlock and what assumptions this relies on, so future refactors don’t inadvertently regress it.
- Now that `TreelandConnector`’s constructor no longer unconditionally sets the VT signal handler, make it explicit in the class contract (e.g., via naming or comments) that `setSignalHandler()` must be called after construction, and consider guarding against multiple calls or ensuring it’s safe to call idempotently.
- Review whether the destructor should explicitly reset or clear the VT signal handler now that it is set later via `setSignalHandler()` rather than in the constructor, to avoid leaving global state in an unexpected configuration on teardown.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This will work as intended, at least for now...
Summary by Sourcery
Bug Fixes: