Skip to content
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

Use a dbus service name as its app_id #921

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aleixpol
Copy link
Contributor

Also decode the unit name for dbus services and offer to the implementation for some extra context of what is going on.

Also decode the unit name for dbus services and offer to the
implementation for some extra context of what is going on.
@aleixpol
Copy link
Contributor Author

I've tested this to run fine on dbus-broker, I'm not sure it works alright with dbus-daemon. It's still something that should be addressed on the daemon itself.

@swick
Copy link
Contributor

swick commented Nov 28, 2022

I wonder if dbus-broker should use the app prefix if there is a corresponding desktop file. The dbus prefixed name seems like an implementation detail we should probably not rely on.

@aleixpol
Copy link
Contributor Author

I'm not sure, do you know who should be asked?

I keep going back-and-forth on this though. I think that ideally it should be the full .service filename, as in org.gnome.Identity.service or org.kde.kdeconnect.service.

As is, it's not giving us much information anyway.

@swick
Copy link
Contributor

swick commented Nov 29, 2022

bus1/dbus-broker#302


g_assert (g_str_has_prefix (unit, "dbus-"));

// Parses a unit name such as dbus-:1.2-org.kde.kdeconnect@7.service
Copy link
Collaborator

@smcv smcv Nov 29, 2022

Choose a reason for hiding this comment

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

This naming convention for systemd units is specific to dbus-broker.

On systems that have systemd activation available, the reference implementation (dbus-daemon) does not guarantee that activated services are in any sort of systemd service or scope: D-Bus services with SystemdService=foo.service are run as foo.service (probably also true for dbus-broker), and D-Bus services without SystemdService are run via "traditional activation", mechanically the same as what happens on non-systemd systems like *BSD or Devuan, so they end up as child processes in dbus.service (this behaviour is unlike dbus-broker).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having said that, if a unit name matches this pattern then you can probably be quite confident that it's named according to how dbus-broker does it.

}
else
{
g_warning("Failed to parse dbus name %s", unit);
Copy link
Collaborator

@smcv smcv Nov 29, 2022

Choose a reason for hiding this comment

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

This will warn on some entirely legitimate unit names. There is a weak convention that a D-Bus service with Name=com.example.Whatever often has SystemdService=dbus-com.example.Whatever.service, which starts with dbus- but does not match dbus-broker's pattern.

I don't know of any concrete examples of user services (session bus services) doing this, but systemd's own dbus-org.freedesktop.login1.service and friends use this pattern for their system services (system bus services).

@ebassi
Copy link
Collaborator

ebassi commented Feb 23, 2023

@aleixpol Could you please rebase this pull request?

@swick
Copy link
Contributor

swick commented Feb 23, 2023

I still believe that we should fix this in dbus-broker instead of adding more non-standard cgroup naming schemes.

@GeorgesStavracas GeorgesStavracas added enhancement needs discussion Needs discussion on how to implement or fix the corresponding task labels Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs discussion Needs discussion on how to implement or fix the corresponding task
Projects
Status: Triaged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants