-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
logind: use SD_BUS_ERROR_ACCESS_DENIED #32951
Conversation
Important An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released. |
src/login/logind-dbus.c
Outdated
@@ -1191,7 +1191,7 @@ static int method_release_session(sd_bus_message *message, void *userdata, sd_bu | |||
return r; | |||
|
|||
if (session != sender_session) | |||
return sd_bus_error_set(error, BUS_ERROR_NOT_IN_CONTROL, "You are not in control of this session"); | |||
return sd_bus_error_set(error, SD_BUS_ERROR_ACCESS_DENIED, "You are not in control of this session"); |
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.
please also change the text, i.e. maybe say "Refused to release session, since it is not yours" or so?
i.e. i think the wording "in control of this session" should be used only for doing session controller related operations, which this doesn't do
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.
or maybe "Refused to release session, since it doesn't match the one of the client"?
5504ef8
to
7658e4a
Compare
Updated.
|
It seems not... |
As requested in post-merge review systemd#32869 (review): > NotInControl error is really about session controllers, but this here really > is different.
7658e4a
to
174657e
Compare
On Tue, May 21, 2024 at 06:46:57AM -0700, Yu Watanabe wrote:
> Updated.
It seems not...
It should be now.
|
The test failures are all over, but don't seem related. |
As requested in post-merge review
#32869 (review):