Bug 40409

Created attachment 50601 [details] [review]
minor bugfix

Created attachment 50602 [details] [review]
the actual activation patch

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.

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

Review of attachment 50601 [details] [review]:

This is not a complete fix, please review Bug #39230 for the complete version.

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.

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.

Created attachment 56494 [details] [review]
Rebased activation patch

Comment on attachment 50601 [details] [review]
minor bugfix

Obsoleted by #39230

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.)

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".

Do you still want this in 1.6?

Yes, totally.

(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).

-- 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.