-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
rootless: set DBUS_SESSION_BUS_ADDRESS if it is not set #4166
rootless: set DBUS_SESSION_BUS_ADDRESS if it is not set #4166
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @edsantiago @mheon @baude |
LGTM |
libpod/runtime.go
Outdated
@@ -387,6 +387,12 @@ func SetXdgDirs() error { | |||
return errors.Wrapf(err, "cannot set XDG_RUNTIME_DIR") | |||
} | |||
|
|||
if rootless.IsRootless() && os.Getenv("DBUS_SESSION_BUS_ADDRESS") == "" { | |||
if _, err := os.Stat(fmt.Sprintf("/run/user/%d/bus", rootless.GetRootlessUID())); err == nil { | |||
os.Setenv("DBUS_SESSION_BUS_ADDRESS", fmt.Sprintf("unix:path=/run/user/%d/bus", rootless.GetRootlessUID())) |
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.
Duplication bothers me; would you mind declaring a local var to hold /run/user/etc
?
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.
I've fixed it in the last commit. I've also dropped the /run/user/%d
part as we already have runtimeDir
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.
Let's fix this and then we can merge
buspath:=fmt.Sprintf("/run/user/%d/bus", rootless.GetRootlessUID()
if _, err := os.Stat(buspath); err := nil {
os.Setenv("DBUS_SESSION_BUS_ADDRESS", fmt.Sprintf("unix:path=%s", buspath)
}
I don't see how this closes either of those bugs. I tried manually setting that envariable, and system tests fail in exactly the same way:
I no longer see the warnings about IOW: this looks like a fine addition but please do not close 4162 or 4164. |
NEVER MIND! My mistake: I missed the Thank you for addressing this so quickly. |
LGTM other than @edsantiago duplication comment. |
if the variable is not set, make sure it has a sane value so that go-dbus won't try to connect to the wrong user session. Closes: containers#4162 Closes: containers#4164 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
fbee8e7
to
57f4149
Compare
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.
/lgtm
if the variable is not set, make sure it has a sane value so that
go-dbus won't try to connect to the wrong user session.
Closes: #4162
Closes: #4164
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com