-
Notifications
You must be signed in to change notification settings - Fork 574
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
many: add support for session dbus services via snaps #2592
Conversation
91bfe4f
to
4e3ea5a
Compare
data/dbus/snapd.conf
Outdated
@@ -0,0 +1,3 @@ | |||
<busconfig> | |||
<servicedir>/var/lib/snapd/desktop/dbus-1/services/</servicedir> | |||
</busconfig> |
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.
IMO, we should remove 'desktop' here-- these aren't desktop files and this isn't limited to 'desktop' systems. Also, I know you probably modeled this name after the directory in /usr/share/dbus-1, but for clarity within snapd, I think this would be better named '/var/lib/snapd/dbus-1/session-services/'. Putting it in /var/lib/snapd/dbus-1 means we can add '/var/lib/snapd/dbus-1/system-services/' if we so choose down the line and keep snappy dbus-y things in one clear spot.
dirs/dirs.go
Outdated
@@ -116,6 +117,7 @@ func SetRootDir(rootdir string) { | |||
SnapMetaDir = filepath.Join(rootdir, snappyDir, "meta") | |||
SnapBlobDir = filepath.Join(rootdir, snappyDir, "snaps") | |||
SnapDesktopFilesDir = filepath.Join(rootdir, snappyDir, "desktop", "applications") | |||
SnapDBusServicesFilesDir = filepath.Join(rootdir, snappyDir, "desktop", "dbus-1/services") |
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 rename as SnapDBusSessionServicesFilesDir to make sure there is no confusion with system services.
wrappers/service.go
Outdated
|
||
baseDir := s.MountDir() | ||
|
||
servicesFiles, err := filepath.Glob(filepath.Join(baseDir, "meta", "gui", "*.service")) |
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 don't think this should be tied to 'gui'. Most session services run in the background and don't have a gui. Some session services are dbus activatable and are started by guis accessing them, but that is a different thing.
wrappers/service.go
Outdated
} | ||
|
||
installedServiceFileName := filepath.Join(dirs.SnapDBusServicesFilesDir, fmt.Sprintf("%s_%s", s.Name(), filepath.Base(df))) | ||
content = sanitizeDBusServiceFile(s, installedServiceFileName, content) |
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 think here, in the MkdirAll above and in the RemoveSnapDBusServiceFiles call below you should think about when we might create a dbus service file for a system service (not that you have to implement that now, but see my later comments). That might be AddSnapDBusServiceFiles and RemoveSnapDBusServiceFiles taking an argument of 'system' or 'session' then picking the directory based on that. It might be something else. A comment in these places explicitly saying this is for session services and that we need to update these to support system services would be nice.
wrappers/desktop_test.go
Outdated
in string | ||
good bool | ||
}{ | ||
{"Activactable=true", false}, |
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.
Typo here. Should be 'Activatable'.
As written, if snapd install finds meta/gui/foo.service, it takes that and creates a session service file in /var/lib/snapd/desktop/dbus-1/services and allows the desktop file to set DBusActivatable. This is simple and it works to start the session service in devmode, but is lacking security policy updates that would allow it to run in strict mode. Also, looking at what can be declared in service files, I think that we can have snapd generate them and expose the options via yaml. The dbus specification (https://dbus.freedesktop.org/doc/dbus-specification.html) is not very clear on service files but bus/desktop-file.h from the dbus sources is. These are the supported directives:
We want to use 'Name=' and 'Exec='. We can ignore 'User=' for now since session services don't use it (they are expected to run as the same user as the user's session) and system services are started as root via systemd (when we support per-snap users, we'll adjust the systemd service file for services, not a dbus serevice service file). We can ignore SystemdService= since it is used by dbus-daemon to tell systemd to start the service on its behalf. We will have dbus-daemon starting session services directly and systemd starting system services directly, so this isn't needed. It is important to remember that on series 16, we have systemd user sessions, but this isn't available in 14.04, so we should have dbus-daemon start session services itself rather than systemd. I propose:
With the above, we have the pieces in place to create session service files, but no way to declare they should be used or how to generate the security policy. I think we can look at the dbus interface and add a slot side attribute. We currently have:
We add:
Then the dbus interface can be adjusted such that:
With this, the |
@jdstrand Thanks for the excellent suggestions. I changed the code as you suggested. Some limitations with this iteration:
|
Fwiw, the 14.04 failure can be ignored, the packaging branch needs an update and then it should work. |
Would it make sense to also generate a If we're assuming systemd for system and user init, then this would reduce the number of code paths for launching confined daemons on the system. |
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.
Nice! A few details and LGTM:
wrappers/desktop.go
Outdated
}, "|")).MatchString | ||
|
||
// rewriteExecLine rewrites a "Exec=" line to use the wrapper path for snap application. | ||
func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) { | ||
func rewriteExecLine(s *snap.Info, desktopFile, line, env string) (string, error) { |
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.
Why the change here? Previous version seemed cleaner than new one. Now the call site needs to know what "env" means (it's not an environment at all) and the curious place in which it will be inserted.
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.
That was from a previous iteration, this is fixed now.
interfaces/dbus/backend.go
Outdated
} | ||
if err := b.setupBusservices(snapInfo, snippets); err != nil { |
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.
Our naming convention is generally:
s/Busconf/BusConf/
s/Busserv/BusServ/
Same below for remove, combineSnippets, addContent, 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.
Thanks, fixed.
interfaces/dbus/backend.go
Outdated
securityTag := appInfo.SecurityTag() | ||
|
||
// FIXME: layer violation, we really should do that in the | ||
// dbus interface but without PR #2613 its really tricky |
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.
#2613 is now in. Seems okay to fix that in a follow up, though, assuming everything else is golden.
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.
Thanks, +1 for a followup
interfaces/dbus/backend.go
Outdated
glob := fmt.Sprintf("%s.service", interfaces.SecurityTagGlob(snapName)) | ||
dir := dirs.SnapDBusSessionServicesFilesDir | ||
if err := os.MkdirAll(dir, 0755); err != nil { | ||
return fmt.Errorf("cannot create directory for DBus service files %q: %s", dir, err) |
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.
The %q/dir is probably redundant here. The error from MkdirAll will include the path name as well.
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.
thanks, removed
interfaces/dbus/backend.go
Outdated
if !ok { | ||
continue | ||
} | ||
if bus != "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.
Shouldn't we allow "system" as well?
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 think we want that in the longer run, my main focus for this PR was to unblock people who need dbus activated services (I think unity and kde has some of those).
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.
If you aren't going to support system now, can you add a TODO. Eg:
// TODO: we can eventually support 'system' by:
// 1. creating the SnapDBusSystemServicesFilesDir directory
// 2. writing the service file to SnapDBusSystemServicesFilesDir
// when 'daemon' is set to 'dbus' (see validate.go)
// 3. add 'Type=dbus' and 'BusName=slot.Attrs["name"].(string)'
// to the systemd unit when 'slot.Attrs["service"].(bool) == True'
// and 'daemon' is set to 'dbus'
data/dbus/snapd.conf
Outdated
@@ -0,0 +1,3 @@ | |||
<busconfig> | |||
<servicedir>/var/lib/snapd/dbus-1/services/</servicedir> |
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.
Can we please use "dbus/services"? That "dbus-1" convention always feels awkward to me, and we don't have many reasons to reproduce it here I think. The compatibility issues and their solution will be the same as for every other service we hold data for.
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.
This will need fixes elsewhere too.
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.
updated.
wrappers/desktop_test.go
Outdated
@@ -300,8 +300,14 @@ func (s *sanitizeDesktopFileSuite) TestLangLang(c *C) { | |||
// bad ones | |||
{"Name[foo=bar", false}, | |||
{"Icon[xx]=bar", false}, | |||
// dbus related | |||
{"Activactable=true", false}, |
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.
Is the typo intended here?
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.
ups, I fixed that typo, the test was focused on the missing "DBus" in front, not so much on the typo :)
interfaces/dbus/backend.go
Outdated
if !ok { | ||
continue | ||
} | ||
isService := slot.Attrs["service"].(bool) |
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.
Even without #2613, can we please at least validate that setting there, and cross-link it here (mentioning this is being used with this purpose).
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 added checks that its really bool and also added a comment why we need this. Should I do more (I'm not sure if that addresses the cross-link comment).
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.
Should be enough, thanks.
@jhenstridge re 'SystemdService key', I covered this in: #2592 (comment) |
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.
Thanks @mvo5 for taking my suggestions and implementing this. +1, but please add the comments I suggested for clarity and next steps.
interfaces/dbus/backend.go
Outdated
return nil | ||
} | ||
|
||
func (b *Backend) setupBusConf(snapInfo *snap.Info, snippets map[string][][]byte) error { |
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.
Can you add a comment for what this function does? Eg:
// Create the DBus busconfig policy for the snap
interfaces/dbus/backend.go
Outdated
return nil | ||
} | ||
|
||
func (b *Backend) removeBusConf(snapName string) error { |
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.
Can you add a comment above this function. Eg
// Remove the DBus busconfig policy for the snap
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.
@mvo5 - nitpick, this comment still didn't make it in. It looks like you added it in the wrong place (see below).
interfaces/dbus/backend.go
Outdated
func (b *Backend) combineSnippets(snapInfo *snap.Info, snippets map[string][][]byte) (content map[string]*osutil.FileState, err error) { | ||
// combineSnippetsBusConf combines security snippets collected from | ||
// all the interfaces affecting a given snap into a content map | ||
// applicable to EnsureDirState. |
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.
Can you rephrase this to be:
// combineSnippetsBusConf combines security snippets for busconfig
// policy collected from all the interfaces affecting a given snap into a
// content map applicable to EnsureDirState.
interfaces/dbus/backend.go
Outdated
} | ||
|
||
return content, nil | ||
} | ||
|
||
func addContent(securityTag string, executableSnippets [][]byte, content map[string]*osutil.FileState) { | ||
func addContentBusConf(securityTag string, executableSnippets [][]byte, content map[string]*osutil.FileState) { |
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.
Can you add a comment above this. Eg:
// addContentBusConf creates/updates the xml busconfig policy for the snap
interfaces/dbus/backend.go
Outdated
if !ok { | ||
continue | ||
} | ||
if bus != "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.
If you aren't going to support system now, can you add a TODO. Eg:
// TODO: we can eventually support 'system' by:
// 1. creating the SnapDBusSystemServicesFilesDir directory
// 2. writing the service file to SnapDBusSystemServicesFilesDir
// when 'daemon' is set to 'dbus' (see validate.go)
// 3. add 'Type=dbus' and 'BusName=slot.Attrs["name"].(string)'
// to the systemd unit when 'slot.Attrs["service"].(bool) == True'
// and 'daemon' is set to 'dbus'
interfaces/dbus/backend.go
Outdated
continue | ||
} | ||
// we check if its a service here so that we know | ||
// if a dbus service file needs to get generated. |
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.
s/to get/to be/
interfaces/dbus/backend.go
Outdated
continue | ||
} | ||
|
||
var buffer bytes.Buffer |
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.
Can you add a comment here. Eg:
// We set only 'Name' and 'Exec' for now. We may add 'User' for 'system'
// services when we support per-snap users. Don't specify 'SystemdService'
// and just let dbus-daemon launch the service since 'SystemdService' is only
// used by dbus-daemon to tell systemd to launch the service and systemd
// user sessions aren't available everywhere yet.
snap/validate.go
Outdated
@@ -134,7 +134,7 @@ var validAppName = regexp.MustCompile("^[a-zA-Z0-9](?:-?[a-zA-Z0-9])*$") | |||
// ValidateApp verifies the content in the app info. | |||
func ValidateApp(app *AppInfo) error { | |||
switch app.Daemon { | |||
case "", "simple", "forking", "oneshot", "dbus", "notify": | |||
case "", "simple", "forking", "oneshot", "notify": |
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 add:
// TODO: add 'dbus' when we support 'service: true' with 'bus: system'
// in the dbus interface
Closing for now as this needs further work |
Reopening @morphis was interessted. |
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.
This is looking really good. Thanks for picking this up! Few open questions inline.
interfaces/dbus/backend.go
Outdated
@@ -74,10 +118,46 @@ func (b *Backend) Setup(snapInfo *snap.Info, opts interfaces.ConfinementOptions, | |||
return nil | |||
} | |||
|
|||
func (b *Backend) setupBusServ(snapInfo *snap.Info, spec interfaces.Specification) error { |
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.
setupBusServ is too generic, no? Right now this is only for session activated services, but the name of this function implies it could be for any dbus service. Perhaps setupBusActivatedSessionServ?
interfaces/dbus/backend.go
Outdated
@@ -86,9 +166,18 @@ func (b *Backend) Remove(snapName string) error { | |||
return nil | |||
} | |||
|
|||
func (b *Backend) removeBusServ(snapName string) error { |
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.
Same here for removeBusServ name being too generic.
interfaces/dbus/backend.go
Outdated
} | ||
return nil | ||
} | ||
|
||
// deriveContent combines security snippets collected from all the interfaces | ||
// affecting a given snap into a content map applicable to EnsureDirState. |
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.
Can you adjust this comment to be:
// deriveContentBusConf combines security snippets for busconfig
// policy collected from all the interfaces affecting a given snap into a
// content map applicable to EnsureDirState.
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.
Thanks, added
dirs/dirs.go
Outdated
@@ -138,6 +140,8 @@ func SetRootDir(rootdir string) { | |||
SnapMetaDir = filepath.Join(rootdir, snappyDir, "meta") | |||
SnapBlobDir = filepath.Join(rootdir, snappyDir, "snaps") | |||
SnapDesktopFilesDir = filepath.Join(rootdir, snappyDir, "desktop", "applications") | |||
SnapDBusSessionServicesFilesDir = filepath.Join(rootdir, snappyDir, "dbus/services") |
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.
Can you add a comment along the lines of:
# Use 'dbus/services' to mirror /usr/share/dbus-1/services for session
# services. With system services, will use 'dbus/system-services'.
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.
Good point, added
@@ -226,6 +227,15 @@ func (iface *DbusInterface) getAttribs(attribs map[string]interface{}) (string, | |||
return bus, name, nil | |||
} | |||
|
|||
// isDbusService returns true if the yaml declares it is a service | |||
func isDbusService(attribs map[string]interface{}) bool { | |||
if isService, ok := attribs["service"].(bool); ok { |
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 don't think "service" is the right term. Anything that slots the dbus interface is a "service", it is just that currently with system services they are automatically started and with session services they are not. If we were going to go with an attribute at all, I would suggest "activatable" and verify that it is only used when "bus" is "session".
That said, we may be able to do better. What if instead of adding an attribute we look at the surrounding yaml. For example, if "bus" is "session" and "daemon" is present, create the session service activation files, otherwise, operate how we do now. This should improve the developer experience and make things "just work" the way people expect.
spec.AddSnippet(strings.Replace(dbusPermanentSlotDBus, old, new, -1)) | ||
} | ||
|
||
// dbus activation currently only supported by the session bus |
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.
and change this to:
Session services get session activation bus policy
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.
👍
// 3. add 'Type=dbus' and 'BusName=slot.Attrs["name"].(string)' to | ||
// the systemd unit when 'slot.Attrs["service"].(bool) == True' and | ||
// 'daemon' is set to 'dbus' | ||
if bus == "session" && isDbusService(slot.Attrs) && len(slot.Apps) == 1 { |
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.
Perhaps use else if
? (nitpick only)
} | ||
|
||
// FIXME: also check that the dbus name is not already taken | ||
// by an existing snap |
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.
This seems important before landing, no?
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.
How can we reliable implement this? Is there any way to query dbus for this as I think examining available configuration files isn't enough. org.freedesktop.DBus.ListActivatableNames looks like a good candidate for this. I am adding godbus to the snapd source tree already with #3260 so would be fine to add it with this PR too.
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.
The FIXME is for if a snap has taken it, not if anything has taken it. snapd has all the information for this: specifically all the snaps on the system with connected dbus interfaces and the attributes of those interfaces.
I don't think we want to query DBus only if we want to make this generalizable beyond activatable names because it would be useful if a non-activatable names were also checked. Right now, those snaps simply fail to start, but we could do something better if we were smarter with existing connections. snapd could additionally look at ListActivatableNames for the activated services to check for non-snap activated service collisions.
interfaces/builtin/dbus.go
Outdated
// by an existing snap | ||
if isDbusService(slot.Attrs) && len(slot.Apps) > 1 { | ||
return fmt.Errorf("cannot add dbus service slot to multiple apps") | ||
} |
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.
This is an interesting requirement because it forces that the snap.yaml must be of the form:
slots:
dbus-session:
...
apps:
foo:
command: ...
slots: [ dbus-session ]
bar:
command: ...
but thus far yaml has allowed specifying slots (and plugs) only in the top-level:
slots:
dbus-session:
...
apps:
foo:
command: ...
bar:
command: ...
where the security policy for 'dbus-session' is given to both foo and bar. What is different here is that there can be only one activation file since we must map the 'Name' in the service file/interface attribute to a specific 'Exec' in the service file. I can't think of a way to make it cleaner than what you have, except to add a comment above saying:
# Because activation service files necessarily must map one DBus well-known
# name to one command, enforce that constraint in the interface here.
I sorta feel like the error message should be improved, but not sure to what. Perhaps add the attribute "name" so it is clearer?
interfaces/dbus/backend.go
Outdated
<servicedir>/var/lib/snapd/dbus/services/</servicedir> | ||
</busconfig> | ||
`) | ||
return ioutil.WriteFile(reexecDbusSessionConf, sessionBusConfig, 0644) |
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.
This checks for snapd.conf and exits, but snapd is not creating it. Is that because snapd distro packaging will always ship this file? If so, then with how the code is written, snapd-reexec.conf will only ever be written if snapd.conf for some reason doesn't exist. While I see that in your postrm you cleanup this file on purge (great!), for systems only upgrading, if a later version of snapd then comes that provides snapd.conf, the previously created snapd-reexec.conf will never get updated or used, potentially causing confusion. Is snapd-reexec.conf meant to be preferred over snapd.conf when re-execing? If so, this code doesn't do that.
How is this supposed to work with packaging? Perhaps a comment in the code saying as much would 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.
A few things I'd like to see changed. Let me know if you need a hand, I will gladly make those changes.
interfaces/dbus/backend.go
Outdated
// packaged snapd. | ||
func setupHostDBusSessionConf() error { | ||
dbusSessionConf := filepath.Join(dirs.SnapSessionBusPolicyDir, "snapd.conf") | ||
if osutil.FileExists(dbusSessionConf) { |
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.
Could we use osutil.EnsureDirState for this? This will allow us to just have a simple call here that ensures the correct content is in that file.
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.
This is done now.
interfaces/dbus/backend.go
Outdated
|
||
content := map[string]*osutil.FileState{} | ||
for securityTag, serviceContent := range spec.(*Specification).SessionServices() { | ||
fname := fmt.Sprintf("%s.service", securityTag) |
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.
Are all security tag valid systemd unit file names?
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.
If not we can pipe the security tag through systemd-escape, which is maybe safe in any case when we generate a systemd unit name.
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.
The files written here are dbus service files. The spec https://dbus.freedesktop.org/doc/dbus-specification.html has no restrictions on the name AFAICS.
interfaces/dbus/backend.go
Outdated
func (b *Backend) setupBusActivatedSessionServ(snapInfo *snap.Info, spec interfaces.Specification) error { | ||
snapName := snapInfo.Name() | ||
|
||
content := map[string]*osutil.FileState{} |
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.
Could you please move this to a helper similar to deriveContentBusConf
?
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 reworked the code to do that now.
interfaces/dbus/spec.go
Outdated
spec.sessionServices[appInfo.SecurityTag()] = fmt.Sprintf(`[D-BUS Service] | ||
Name=%s | ||
Exec=%s | ||
`, name, appInfo.LauncherCommand()) |
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.
Note that we have systemd.Service
and the systemd
backend as well. We could perhaps reuse some of those here.
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.
In this particular case we write a dbus service file which is different from a systemd unit file.
see discussion in #3323 whether we need to tweak wrappers.IsService here |
@pedronis I think it does make sense to make wrapper.IsService() return true for dbus activated services. This way we also avoid writing the /snap/bin/svc binary and avoid polluting the namespace of PATH with those. |
Closing for now as this needs more work but there is no time available right now, I will reopen once I get the chance to work on it again. |
@mvo5 Did you get a chance to work on it again? 😃 |
@PixsaOJ: D-Bus activation support is available in snapd. Support for session bus services is currently gated behind an experimental feature flag. You can see some more details here: https://forum.snapcraft.io/t/enabling-user-daemons-and-d-bus-activation/22318?u=jamesh |
This adds support for dbus activation for snaps. This version is session bus only but it will be relatively easy to extend for the system bus. It implements the proposal from Jamie in #2592 (comment) and the snap.yaml for an application that implements io.snapcraft.SnapDbusService will look something like this (
service: true
is the key here):LP: #1648990