Skip to content

Add SetType method to logind Session interface#14925

Merged
poettering merged 1 commit intosystemd:masterfrom
electrickite:set-session-type
Apr 30, 2020
Merged

Add SetType method to logind Session interface#14925
poettering merged 1 commit intosystemd:masterfrom
electrickite:set-session-type

Conversation

@electrickite
Copy link
Contributor

Adds a new method, SetType, to the logind Session interface. SetType allows the session type to be changed, but only by the session's current controller. If TakeControl has not been called, the method will fail.

In addition, the session type will be reset to its original value once control is released. This should help prevent a session from entering an inconsistent state, for example if the controller crashes.

The sequence to change type is: TakeControl -> SetType. Calling ReleaseControl or otherwise losing session control will reset the Type to its original value.

The method signature is: SetType(in s arg_0) where arg_0 is the session type name.

This should resolve #14489

@yuwata yuwata added the login label Feb 25, 2020
@keszybz keszybz added this to the v246 milestone Mar 5, 2020
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation-wise this looks good. Nevertheless, I'd prefer to wait for @poettering's ack before merging. This PR is assigned to v246, so let's wait for now, it'll not be forgotten.

@keszybz keszybz requested a review from poettering March 9, 2020 21:40
Copy link
Member

@poettering poettering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the late review.

looks excellent, some minor comments.

Also, please squash your commits if later commits just fix up earlier commits. i.e. we generally want "perfect" PRs, where each commit is a logical step, but not necessarily a historical one, and everything remains perfectly bisectable. i

@poettering
Copy link
Member

also needs a rebase

@poettering poettering added needs-rebase reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 28, 2020
@poettering
Copy link
Member

looks great, just two minor nitpicks.

@emersion
Copy link
Contributor

Compositor impl question: should the compositor set XDG_SESSION_TYPE itself when calling SetType?

@poettering
Copy link
Member

@emersion hmm, you mean the env var? set for what processes precisely? for itself? if the compositor uses SetType to change the type and then goes on and forks off other stuff that is supposed to be part of the same session then yes it probably makes sense to override the env var too, so that things stay in sync.

@emersion
Copy link
Contributor

hmm, you mean the env var? set for what processes precisely? for itself?

Yes, the env var, for other processes.

yes it probably makes sense to override the env var too, so that things stay in sync

Ack, thanks.

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed needs-rebase reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 30, 2020
@poettering
Copy link
Member

lgtm

@electrickite
Copy link
Contributor Author

I'm not sure why the bionic-i386 test is failing here, but the errors in the log don't seem related to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed login

Development

Successfully merging this pull request may close these issues.

Allow logind session type to change

5 participants