Bug 40409 - [PATCH] Natively read systemd unit directories to find actvitable services
Summary: [PATCH] Natively read systemd unit directories to find actvitable services
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-08-26 14:30 UTC by Lennart Poettering
Modified: 2018-10-12 21:12 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
add _dbus_stat_is_file() (2.33 KB, patch)
2011-08-26 14:30 UTC, Lennart Poettering
Details | Splinter Review
minor bugfix (878 bytes, patch)
2011-08-26 14:31 UTC, Lennart Poettering
Details | Splinter Review
the actual activation patch (31.45 KB, patch)
2011-08-26 14:32 UTC, Lennart Poettering
Details | Splinter Review
add _dbus_stat_is_file() (2.33 KB, patch)
2011-08-26 14:35 UTC, Lennart Poettering
Details | Splinter Review
_dbus_stat_is_file() with space before parenthesis fix (2.33 KB, patch)
2012-02-01 20:26 UTC, Lennart Poettering
Details | Splinter Review
Rebased activation patch (31.37 KB, patch)
2012-02-01 20:38 UTC, Lennart Poettering
Details | Splinter Review

Description Lennart Poettering 2011-08-26 14:30:55 UTC
Created attachment 50600 [details] [review]
add _dbus_stat_is_file()

This greatly simplifies how people can make their units bus activatable.

See patch descriptions for more details.
Comment 1 Lennart Poettering 2011-08-26 14:31:46 UTC
Created attachment 50601 [details] [review]
minor bugfix
Comment 2 Lennart Poettering 2011-08-26 14:32:13 UTC
Created attachment 50602 [details] [review]
the actual activation patch
Comment 3 Lennart Poettering 2011-08-26 14:35:27 UTC
Created attachment 50604 [details] [review]
add _dbus_stat_is_file()

hmm, first version of _dbus_stat_is_file() was slightly borked, here's another try for that patch.
Comment 4 Simon McVittie 2011-08-30 07:26:15 UTC
Review of attachment 50604 [details] [review]:

This one looks fine, apart from a cosmetic change.

::: dbus/dbus-sysdeps-util-unix.c
@@ +620,3 @@
+_dbus_stat_is_file (DBusStat *statbuf)
+{
+  return S_ISREG(statbuf->mode);

Space before parenthesis
Comment 5 Simon McVittie 2011-08-30 07:27:39 UTC
Review of attachment 50601 [details] [review]:

This is not a complete fix, please review Bug #39230 for the complete version.
Comment 6 Simon McVittie 2011-08-30 07:37:39 UTC
Review of attachment 50602 [details] [review]:

> This patch makes it unnecessary to ship the D-Bus
> activation file

Only on operating systems where systemd is used for system activation - meaning that upstreams of portable projects need to ship a D-Bus-style service file anyway...

Please respond to the thread I started on the D-Bus mailing list, subject "D-Bus system services: systemd, Upstart, the future". I'm not going to merge any changes to activation support (either this or Upstart) until we have a plan for how this is going to look.
Comment 7 Lennart Poettering 2012-02-01 20:26:43 UTC
Created attachment 56493 [details] [review]
_dbus_stat_is_file() with space before parenthesis fix

(In reply to comment #4)
> Review of attachment 50604 [details] [review] [review]:
> 
> This one looks fine, apart from a cosmetic change.
> 
> ::: dbus/dbus-sysdeps-util-unix.c
> @@ +620,3 @@
> +_dbus_stat_is_file (DBusStat *statbuf)
> +{
> +  return S_ISREG(statbuf->mode);
> 
> Space before parenthesis

Fixed.
Comment 8 Lennart Poettering 2012-02-01 20:38:16 UTC
Created attachment 56494 [details] [review]
Rebased activation patch
Comment 9 Lennart Poettering 2012-02-09 13:52:41 UTC
Comment on attachment 50601 [details] [review]
minor bugfix

Obsoleted by #39230
Comment 10 Simon McVittie 2012-02-10 03:54:25 UTC
Comment on attachment 56494 [details] [review]
Rebased activation patch

Review of attachment 56494 [details] [review]:
-----------------------------------------------------------------

> This patch makes it unnecessary to ship the D-Bus
> activation file

I still think this should say "... in distributions like Fedora where only systemd is supported". Upstreams aiming to be more portable than systemd (i.e. most of them), and packagers for distributions with both systemd and ye olde init, still need a D-Bus .service file too.

::: bus/activation.c
@@ +511,5 @@
> +
> +  if (!_dbus_stat (&file_path, &stat_buf, NULL))
> +    {
> +      dbus_set_error (error, DBUS_ERROR_FAILED,
> +                      "Can't stat the service file\n");

Unnecessary \n.

@@ +515,5 @@
> +                      "Can't stat the service file\n");
> +      goto out;
> +    }
> +
> +  if (!_dbus_stat_is_file (&stat_buf))

If this is relying on _dbus_stat() remaining stat() and never becoming lstat(), please augment the documentation of _dbus_stat(): "On platforms with symlinks, this function should follow them (it is stat, not lstat). systemd activation relies on this".

@@ +533,5 @@
> +
> +      /* Strip prefix and suffix of the file name. The caller will
> +         have verified them for us, so we just blindly cut them off
> +         here to get the bus name. This turns
> +         "dbus-org.foobar.Waldo.service into org.foobar.Waldo. */

Could benefit from some double quotes so it's clear that the end-of-sentence full stop is not part of the literal string "org.foobar.Waldo"

@@ +535,5 @@
> +         have verified them for us, so we just blindly cut them off
> +         here to get the bus name. This turns
> +         "dbus-org.foobar.Waldo.service into org.foobar.Waldo. */
> +      len = _dbus_string_get_length (filename);
> +      name = _dbus_memdup (_dbus_string_get_const_data (filename) + 5, len - 5 - 8 + 1);

I'd really prefer this to use checked DBusString operations, as per the coding style described in HACKING:

DBusString name;

if (!_dbus_string_init (&name))
  /* OOM */

if (!_dbus_string_copy_len (filename, 5, len - 5 - 8, &name, 0))
  /* OOM */

and then use name's const_data for the hash table lookup etc.

This implementation looks right, but is unnecessarily clever: dbus-daemon is security-sensitive, so it's not enough to be right, it also has to be *obviously* right.

@@ +561,5 @@
> +        }
> +
> +      entry->refcount = 1;
> +      entry->s_dir = s_dir;
> +      entry->name = name;

This would have to use _dbus_string_steal_data().

@@ +565,5 @@
> +      entry->name = name;
> +
> +      /* The filename and systemd service name in this case are the
> +         same */
> +      entry->systemd_service = _dbus_strdup (_dbus_string_get_const_data (filename));

_dbus_string_copy_data does both of these together

@@ +601,5 @@
> +  retval = TRUE;
> +
> +out:
> +  /* if these have been transferred into entry, the variables will be NULL */
> +  _dbus_string_free (&file_path);

I don't think that comment is meaningful any more.

(You'll need to _dbus_string_free (&name) here too, though.)

@@ +930,5 @@
> +          BUS_SET_OOM (error);
> +          goto out;
> +        }
> +
> +      if (!load_systemd_file_entry (activation, s_dir, &filename, &tmp_error))

Couldn't you pass full_path to this, avoiding recomputing it inside l_s_f_e()?

@@ +2093,4 @@
>            return FALSE;
>          }
>  
> +      if (entry->exec)

Could do with a comment, perhaps: "NULL for systemd entries" or something.

I assume you've audited everywhere else that dereferences ->exec?

@@ +2754,5 @@
>  
>    if (!_dbus_list_append (&directories, _dbus_string_get_data (dir)))
>      return FALSE;
>  
> +  activation = bus_activation_new (NULL, &address, &directories, &systemd_directories, NULL);

This looks like the obvious place to check that a systemd service can be loaded from test/data/systemd-services/dbus-com.example.SystemdService.service or something: so please do.

(You'll need to either make it a regular file rather than a symlink, or create the symlink at build time, because Windows.)
Comment 11 Simon McVittie 2012-02-10 03:55:35 UTC
I like the design/refactoring in this patch, although I'd have found it easier to review if it was split into "do the refactoring" and "and now add more systemd".
Comment 12 Simon McVittie 2012-02-24 02:42:43 UTC
Do you still want this in 1.6?
Comment 13 Lennart Poettering 2012-03-05 11:35:14 UTC
Yes, totally.
Comment 14 Simon McVittie 2012-03-14 09:18:00 UTC
(In reply to comment #13)
> Yes, totally.

The answer I was hoping for was more like "yes, here's an updated patch" or "yes, here's a response to your code review" or "no, don't block on it for 1.6". I'm afraid my availability to work on D-Bus is going to reduce significantly in the near future, so you might have to find someone else to review stuff / do the 1.6 release if you want it to happen.

Sorry, but I'm not going to redo/write-tests-for other people's patches with this many of my patches already gathering dust in bugzilla. In particular, I'd like dbus-launch to not suck in 1.6 (Bug #39196, Bug #39197, which are branches of Bug #9884).
Comment 15 GitLab Migration User 2018-10-12 21:12:41 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/55.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.