Bug 99825 - Add support for transient D-Bus session services
Summary: Add support for transient D-Bus session services
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other Linux (All)
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 99873 99874 99876
  Show dependency treegraph
 
Reported: 2017-02-15 12:46 UTC by Simon McVittie
Modified: 2017-02-21 18:39 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
config-parser: Eliminate duplicate functionality (1.99 KB, patch)
2017-02-15 13:32 UTC, Simon McVittie
Details | Splinter Review
config-parser: Fix indentation (933 bytes, patch)
2017-02-15 13:32 UTC, Simon McVittie
Details | Splinter Review
sysdeps: Add accessor for a list of transient service directories (6.12 KB, patch)
2017-02-15 13:33 UTC, Simon McVittie
Details | Splinter Review
config-parser: Add transient service directories (2.34 KB, patch)
2017-02-15 13:34 UTC, Simon McVittie
Details | Splinter Review
test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as an argument (5.14 KB, patch)
2017-02-15 13:34 UTC, Simon McVittie
Details | Splinter Review
sd-activation test: Use a struct for the test context (5.61 KB, patch)
2017-02-15 13:34 UTC, Simon McVittie
Details | Splinter Review
tests: Wrap file-deletion functions to handle EINTR (3.45 KB, patch)
2017-02-15 13:35 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Wait for the killed process to exit (1.28 KB, patch)
2017-02-15 13:35 UTC, Simon McVittie
Details | Splinter Review
test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere harmless (1.47 KB, patch)
2017-02-15 13:36 UTC, Simon McVittie
Details | Splinter Review
sd-activation test: Create and destroy a temporary XDG_RUNTIME_DIR (3.50 KB, patch)
2017-02-15 13:36 UTC, Simon McVittie
Details | Splinter Review
sd-activation test: Exercise transient services (11.99 KB, patch)
2017-02-15 13:37 UTC, Simon McVittie
Details | Splinter Review
sd-activation test: Use a struct for the test context (5.61 KB, patch)
2017-02-17 19:03 UTC, Simon McVittie
Details | Splinter Review
activation: Put activation directories in an ordered list (4.29 KB, patch)
2017-02-17 19:04 UTC, Simon McVittie
Details | Splinter Review
activation test: Use more realistic bus names for services (1002 bytes, patch)
2017-02-17 19:05 UTC, Simon McVittie
Details | Splinter Review
test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as an argument (5.16 KB, patch)
2017-02-17 19:05 UTC, Simon McVittie
Details | Splinter Review
tests: Wrap file-deletion functions to handle EINTR (3.48 KB, patch)
2017-02-17 19:06 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Wait for the killed process to exit (1.28 KB, patch)
2017-02-17 19:07 UTC, Simon McVittie
Details | Splinter Review
test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere harmless (1.47 KB, patch)
2017-02-17 19:08 UTC, Simon McVittie
Details | Splinter Review
config-parser: Remove dead code from system service dirs test (3.52 KB, patch)
2017-02-17 19:09 UTC, Simon McVittie
Details | Splinter Review
config-parser: Simplify test for standard session service dirs (1.76 KB, patch)
2017-02-17 19:09 UTC, Simon McVittie
Details | Splinter Review
config-parser: Don't use dbus_setenv() to test service directories (6.24 KB, patch)
2017-02-17 19:10 UTC, Simon McVittie
Details | Splinter Review
config-parser test: Exercise the full config-parser (6.16 KB, patch)
2017-02-17 19:11 UTC, Simon McVittie
Details | Splinter Review
sd-activation test: Create and destroy a temporary XDG_RUNTIME_DIR (3.51 KB, patch)
2017-02-17 19:12 UTC, Simon McVittie
Details | Splinter Review
bus_config_parser_get_watched_dirs: Turn into a helper function (5.67 KB, patch)
2017-02-17 19:13 UTC, Simon McVittie
Details | Splinter Review
activation-helper: Rename bus_config_parser_get_service_dirs (2.58 KB, patch)
2017-02-17 19:13 UTC, Simon McVittie
Details | Splinter Review
config-parser: Store service directories in structs (14.10 KB, patch)
2017-02-17 19:15 UTC, Simon McVittie
Details | Splinter Review
activation: Add support for enforcing strict naming on .service files (5.70 KB, patch)
2017-02-17 19:19 UTC, Simon McVittie
Details | Splinter Review
sysdeps: Add accessor for a list of transient service directories (8.08 KB, patch)
2017-02-17 19:21 UTC, Simon McVittie
Details | Splinter Review
config-parser: Add transient service directories (8.20 KB, patch)
2017-02-17 19:22 UTC, Simon McVittie
Details | Splinter Review
sd-activation test: Exercise transient services (11.99 KB, patch)
2017-02-17 19:24 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Windows: add errno.h for ENOENT, skip EINTR checks (2.10 KB, patch)
2017-02-17 19:39 UTC, Simon McVittie
Details | Splinter Review
spec: Don't say implementation-specific locations must be lowest priority (1.81 KB, patch)
2017-02-17 20:58 UTC, Simon McVittie
Details | Splinter Review
dbus-daemon(1): Describe how session services are found (3.54 KB, patch)
2017-02-17 20:59 UTC, Simon McVittie
Details | Splinter Review
[1] config-parser: Simplify test for standard session service dirs (1.86 KB, patch)
2017-02-20 18:50 UTC, Simon McVittie
Details | Splinter Review
[2] config-parser: Don't use dbus_setenv() to test service directories (6.42 KB, patch)
2017-02-20 18:51 UTC, Simon McVittie
Details | Splinter Review
[3] config-parser test: Exercise the full config-parser (5.88 KB, patch)
2017-02-20 18:52 UTC, Simon McVittie
Details | Splinter Review
[4] tests: Consistently don't try to kill pid 0 (1.55 KB, patch)
2017-02-20 18:52 UTC, Simon McVittie
Details | Splinter Review
[5] test-utils-glib: Wait for the killed process to exit (1.39 KB, patch)
2017-02-20 18:53 UTC, Simon McVittie
Details | Splinter Review
[6] sd-activation test: Create and destroy a temporary XDG_RUNTIME_DIR (3.71 KB, patch)
2017-02-20 18:54 UTC, Simon McVittie
Details | Splinter Review
[7] bus_config_parser_get_watched_dirs: Turn into a helper function (6.02 KB, patch)
2017-02-20 18:56 UTC, Simon McVittie
Details | Splinter Review
[8] activation-helper: Rename bus_config_parser_get_service_dirs (2.84 KB, patch)
2017-02-20 18:57 UTC, Simon McVittie
Details | Splinter Review
[9] config-parser: Store service directories in structs (13.57 KB, patch)
2017-02-20 18:58 UTC, Simon McVittie
Details | Splinter Review
[10] activation: Add support for enforcing strict naming on .service files (6.49 KB, patch)
2017-02-20 19:01 UTC, Simon McVittie
Details | Splinter Review
[10] activation: Add support for enforcing strict naming on .service files (6.94 KB, patch)
2017-02-20 19:33 UTC, Simon McVittie
Details | Splinter Review
[11] sysdeps: Add accessor for a list of transient service directories (8.13 KB, patch)
2017-02-20 19:35 UTC, Simon McVittie
Details | Splinter Review
[12] config-parser: Add transient service directories (8.71 KB, patch)
2017-02-20 19:36 UTC, Simon McVittie
Details | Splinter Review
[13] sd-activation test: Exercise transient services (12.01 KB, patch)
2017-02-20 19:38 UTC, Simon McVittie
Details | Splinter Review
[14] spec: Don't say implementation-specific locations must be lowest priority (2.03 KB, patch)
2017-02-20 19:38 UTC, Simon McVittie
Details | Splinter Review
[15] dbus-daemon(1): Describe how session services are found (4.78 KB, patch)
2017-02-20 19:50 UTC, Simon McVittie
Details | Splinter Review
dbus-daemon(1): Describe how session and system services are found (10.09 KB, patch)
2017-02-21 11:55 UTC, Simon McVittie
Details | Splinter Review
test: Create XDG_RUNTIME_DIR with restrictive permissions (1.03 KB, patch)
2017-02-21 12:56 UTC, Simon McVittie
Details | Splinter Review
sd-activation test: Don't declare unused variable with old AppArmor (966 bytes, patch)
2017-02-21 12:57 UTC, Simon McVittie
Details | Splinter Review
Add a simple integration test for transient services (10.18 KB, patch)
2017-02-21 17:20 UTC, Simon McVittie
Details | Splinter Review
Add a simple integration test for transient services (10.10 KB, patch)
2017-02-21 18:05 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-02-15 12:46:09 UTC
systemd --user puts some transient service directories on a tmpfs in its search path for .service files. These can be used by session management tools to make systemd user services magically appear at runtime. Their priority is higher than /usr/lib/systemd/user but lower than /etc/systemd/user.

It would be nice to have the same feature in dbus-daemon, adding services with a priority higher than $XDG_DATA_HOME:$XDG_DATA_DIRS[1]. For instance, with the move towards DBusActivatable applications using GApplication or equivalent, a lot of desktop applications are adding D-Bus session services with "ExecStart=/usr/bin/whatever --gapplication-service". We could potentially define extra keys[2] in .desktop files that cause some helper process - perhaps a session manager like gnome-session, or a systemd generator - to synthesize corresponding D-Bus session services for those .desktop files, instead of each application having to ship a session service.

The patches that I'm about to attach do not add a similar feature for the system bus, because that isn't currently my focus, but that could be a future enhancement.

---

[1] And lower than any "configuration" layer that we might add in the future, perhaps using {$XDG_CONFIG_HOME:$XDG_CONFIG_DIRS}/dbus-1/services - but we've never had that and I don't intend to add it now.

[2] Extra keys are needed because a .desktop file's Exec= is defined to be something that will do the equivalent of org.freedesktop.Application.Activate() - but the Exec for a D-Bus service (or for that matter the ExecStart for a systemd service) needs to be something that will start the process but not activate it, like --gapplication-service for executables that use GApplication.
Comment 1 Simon McVittie 2017-02-15 12:59:24 UTC
Design - minimum viable implementation
======================================

If <standard_session_servicedirs/> isn't in the bus configuration, do not provide this feature - I'm effectively treating this feature as a new standard session service directory.

If XDG_RUNTIME_DIR is not set, or on Windows, quietly do nothing: this feature does not exist on such platforms.

If XDG_RUNTIME_DIR does not exist, does not belong to the user, or is writeable by anyone other than the user[1], or if XDG_RUNTIME_DIR/dbus-1 or XDG_RUNTIME_DIR/dbus-1/services cannot be created, does not belong to the user, or is writeable by anyone other than the user[1], log a warning and do nothing: this feature is unavailable until that misconfiguration is fixed.

If everything is OK, put XDG_RUNTIME_DIR/dbus-1/services in the search path as the first of the standard session service directories (before XDG_DATA_HOME).

During startup, if dbus-daemon is configured to use XDG_RUNTIME_DIR/dbus-1/services, it should create that directory.

If that directory is created before dbus-daemon comes up (perhaps by a systemd generator that also pre-populates it), that is also considered valid.

If another process drops .service files into XDG_RUNTIME_DIR/dbus-1/services while the dbus-daemon might be running, it is expected to instruct the dbus-daemon to reload by calling the ReloadConfig() method. If drops .service files into XDG_RUNTIME_DIR/dbus-1/services while the dbus-daemon is not running yet (like a systemd generator), it is not necessary to do that.

[1] This is just paranoia, really. I did it because I was going to call stat() anyway.

Design - "nice to have"
=======================

We should perhaps not put an inotify watch on XDG_RUNTIME_DIR/dbus-1/services, because the inotify watches on other service and configuration directories were arguably "too much magic"; we can't stop inotifying on the current directories (at least not without a configuration change) because that would break the expectations of current code, but we can at least minimize magic for the future.

Similarly, we should perhaps require services found in XDG_RUNTIME_DIR/dbus-1/services to have the canonical name ${bus_name}.service, even though we do not enforce that for other session services because it would break existing code.

Both of these require a way to distinguish between directories in the search path, making it a more complex data structure than a simple list of strings. I haven't shaved these particular yaks yet.
Comment 2 Simon McVittie 2017-02-15 13:32:14 UTC
Created attachment 129626 [details] [review]
config-parser: Eliminate duplicate functionality

We had two ways to append a path to the list of service directories.
Collapse them into one.
Comment 3 Simon McVittie 2017-02-15 13:32:26 UTC
Created attachment 129627 [details] [review]
config-parser: Fix indentation
Comment 4 Simon McVittie 2017-02-15 13:33:28 UTC
Created attachment 129628 [details] [review]
sysdeps: Add accessor for a list of transient service  directories
Comment 5 Simon McVittie 2017-02-15 13:34:00 UTC
Created attachment 129629 [details] [review]
config-parser: Add transient service directories

For configuration purposes these are treated as part of the standard
session service directories, to avoid having to add new configuration
syntax which would prevent an old dbus-daemon from reloading
successfully. From an API perspective, they're separate, though.
Comment 6 Simon McVittie 2017-02-15 13:34:14 UTC
Created attachment 129630 [details] [review]
test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as  an argument
Comment 7 Simon McVittie 2017-02-15 13:34:52 UTC
Created attachment 129631 [details] [review]
sd-activation test: Use a struct for the test context

This is going to be necessary to be able to influence setup() and
teardown() as well as just the individual tests.
Comment 8 Simon McVittie 2017-02-15 13:35:17 UTC
Created attachment 129632 [details] [review]
tests: Wrap file-deletion functions to handle EINTR

The GLib functions we're using don't, and it seems to be possible to be
interrupted during cleanup for our tests.
Comment 9 Simon McVittie 2017-02-15 13:35:51 UTC
Created attachment 129633 [details] [review]
test-utils-glib: Wait for the killed process to exit

Otherwise, removing transient service directories that are being
watched by the dbus-daemon can fail with EAGAIN.
Comment 10 Simon McVittie 2017-02-15 13:36:08 UTC
Created attachment 129634 [details] [review]
test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere  harmless

We want to be able to use <standard_system_datadirs/> in tests
without picking up someone else's .service files.
Comment 11 Simon McVittie 2017-02-15 13:36:25 UTC
Created attachment 129635 [details] [review]
sd-activation test: Create and destroy a temporary  XDG_RUNTIME_DIR
Comment 12 Simon McVittie 2017-02-15 13:37:43 UTC
Created attachment 129636 [details] [review]
sd-activation test: Exercise transient services

To do this, we have to use the <standard_session_servicedirs/>.
A previous commit ensured that those don't provide any service files
we don't expect.

---

That's all for now.

To be added (or at least considered) before merge:

* avoid having inotify watch the transient service directory
* be strict about .service file names there
Comment 13 Philip Withnall 2017-02-15 14:18:43 UTC
Comment on attachment 129626 [details] [review]
config-parser: Eliminate duplicate functionality

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

++
Comment 14 Philip Withnall 2017-02-15 14:19:12 UTC
Comment on attachment 129627 [details] [review]
config-parser: Fix indentation

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

++

Would have been better if you’d re-indented using Unicode spaces.
Comment 15 Philip Withnall 2017-02-15 14:29:22 UTC
Comment on attachment 129628 [details] [review]
sysdeps: Add accessor for a list of transient service  directories

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

A few questions.

::: dbus/dbus-sysdeps-util-unix.c
@@ +1201,5 @@
> +  /* This succeeds on EEXIST */
> +  if (create && !_dbus_create_directory (string, error))
> +    return FALSE;
> +
> +  if (stat (dir, &buf) != 0)

Do you want to use lstat() here and check that it’s not a symbolic link which might be controlled by an attacker?

@@ +1281,5 @@
> +    }
> +
> +  xdg_runtime_dir = _dbus_getenv ("XDG_RUNTIME_DIR");
> +
> +  /* Not an error, we just can't have transient session services */

Maybe output a debug message to note this. It’s going to be a fairly rare occurrence, right?
Comment 16 Philip Withnall 2017-02-15 14:30:35 UTC
Comment on attachment 129629 [details] [review]
config-parser: Add transient service directories

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

Looks good. A couple of questions.

::: bus/config-parser.c
@@ +837,5 @@
> +      if (_dbus_set_up_transient_session_servicedirs (&dirs, &local_error))
> +        {
> +          while ((link = _dbus_list_pop_first_link (&dirs)))
> +            service_dirs_append_link_unique_or_free (&parser->service_dirs,
> +                                                     link);

Is it deliberate that this reverses the order of the list?

@@ +842,5 @@
> +        }
> +      else
> +        {
> +          /* Failing to set these up isn't fatal */
> +          _dbus_warn ("Unable to set up transient service directory: %s",

Nitpick: s/directory/directories/? Not sure what’s more appropriate here. I’m fine with either, really.
Comment 17 Philip Withnall 2017-02-15 14:33:31 UTC
Comment on attachment 129630 [details] [review]
test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as  an argument

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

::: test/test-utils-glib.c
@@ +101,5 @@
>    GError *error = NULL;
>    GString *address;
>    gint address_fd;
>    GPtrArray *argv;
> +  gchar **envp = g_get_environ ();

Technically this will leak on all the early return paths. Might be simpler to just assign it immediately before the g_environ_setenv() call.
Comment 18 Philip Withnall 2017-02-15 14:35:34 UTC
Comment on attachment 129631 [details] [review]
sd-activation test: Use a struct for the test context

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

Two tiny nitpicks but this otherwise looks good.

::: test/sd-activation.c
@@ +828,5 @@
> +    { "com.example.SendDeniedByAppArmorLabel" },
> +    { "com.example.SendDeniedByNonexistentAppArmorLabel" },
> +    { "com.example.SendDeniedByAppArmorName" },
> +#endif
> +    { "com.example.SendDenied" }

Nitpick: Add a trailing comma to eliminate future diff noise.

@@ +836,5 @@
> +{
> +#if defined(DBUS_TEST_APPARMOR_ACTIVATION)
> +    { "com.example.ReceiveDeniedByAppArmorLabel" },
> +#endif
> +    { "com.example.ReceiveDenied" }

Nitpick: Add a trailing comma to eliminate future diff noise.
Comment 19 Simon McVittie 2017-02-15 15:15:29 UTC
(In reply to Philip Withnall from comment #15)
> Do you want to use lstat() here and check that it’s not a symbolic link
> which might be controlled by an attacker?

I think I don't, because:

* we're guarding against accidents (running under sudo without clearing
  the environment), not malice
* if we later decide that it should be canonically
  $XDG_RUNTIME_DIR/dbus-1/transient-services or something, with a symlink
  to the old name, I don't really want to break that for no reason

> > +  xdg_runtime_dir = _dbus_getenv ("XDG_RUNTIME_DIR");
> > +
> > +  /* Not an error, we just can't have transient session services */
> 
> Maybe output a debug message to note this. It’s going to be a fairly rare
> occurrence, right?

In practice we will have a suitable XDG_RUNTIME_DIR if we are on Linux with systemd-logind, but not if we are on OS X, *BSD, older Linux, Linux whose users hate systemd, etc.

dbus' only debug messages are _dbus_verbose(), which is normally compiled out, and so horribly spammy that they're of little use. I suppose adding one here wouldn't hurt, but I'm reluctant to add them without good reason.
Comment 20 Simon McVittie 2017-02-15 15:18:11 UTC
(In reply to Philip Withnall from comment #16)
> > +          while ((link = _dbus_list_pop_first_link (&dirs)))
> > +            service_dirs_append_link_unique_or_free (&parser->service_dirs,
> > +                                                     link);
> 
> Is it deliberate that this reverses the order of the list?

It... doesn't? DBusList can add or remove from either end in O(1). We repeatedly take the first thing in dirs and make it the new last thing in service_dirs, so I think we're preserving order?

> > +          /* Failing to set these up isn't fatal */
> > +          _dbus_warn ("Unable to set up transient service directory: %s",
> 
> Nitpick: s/directory/directories/? Not sure what’s more appropriate here.
> I’m fine with either, really.

There's currently up to 1. They'll only multiply if we decide we want some sort of hierarchy of transient directories (which systemd does have, because generators don't write to the same directories as ordinary transient services, but I think that's probably more complexity than we need).
Comment 21 Simon McVittie 2017-02-15 15:18:27 UTC
(In reply to Philip Withnall from comment #17)
> Technically this will leak on all the early return paths. Might be simpler
> to just assign it immediately before the g_environ_setenv() call.

Good catch, will fix that.
Comment 22 Simon McVittie 2017-02-15 15:19:42 UTC
(In reply to Philip Withnall from comment #18)
> Nitpick: Add a trailing comma to eliminate future diff noise.

That isn't ISO C89, and as far as I'm aware dbus is still stuck in the 80s for Microsoft Visual C++'s benefit. I mitigated this by putting the oldest/most basic one last, so we can insert new entries above it.
Comment 23 Philip Withnall 2017-02-15 16:07:12 UTC
(In reply to Simon McVittie from comment #19)
> (In reply to Philip Withnall from comment #15)
> > Do you want to use lstat() here and check that it’s not a symbolic link
> > which might be controlled by an attacker?
> 
> I think I don't, because:
> 
> * we're guarding against accidents (running under sudo without clearing
>   the environment), not malice
> * if we later decide that it should be canonically
>   $XDG_RUNTIME_DIR/dbus-1/transient-services or something, with a symlink
>   to the old name, I don't really want to break that for no reason

OK. Could do with a comment about the first point in the code.

> > > +  xdg_runtime_dir = _dbus_getenv ("XDG_RUNTIME_DIR");
> > > +
> > > +  /* Not an error, we just can't have transient session services */
> > 
> > Maybe output a debug message to note this. It’s going to be a fairly rare
> > occurrence, right?
> 
> In practice we will have a suitable XDG_RUNTIME_DIR if we are on Linux with
> systemd-logind, but not if we are on OS X, *BSD, older Linux, Linux whose
> users hate systemd, etc.

Might also be worth adding that as a comment.

> dbus' only debug messages are _dbus_verbose(), which is normally compiled
> out, and so horribly spammy that they're of little use. I suppose adding one
> here wouldn't hurt, but I'm reluctant to add them without good reason.

I’d be in favour of adding one; I could see it highlighting a thinko if someone’s debugging something.
Comment 24 Philip Withnall 2017-02-15 16:08:19 UTC
(In reply to Simon McVittie from comment #20)
> (In reply to Philip Withnall from comment #16)
> > > +          while ((link = _dbus_list_pop_first_link (&dirs)))
> > > +            service_dirs_append_link_unique_or_free (&parser->service_dirs,
> > > +                                                     link);
> > 
> > Is it deliberate that this reverses the order of the list?
> 
> It... doesn't? DBusList can add or remove from either end in O(1). We
> repeatedly take the first thing in dirs and make it the new last thing in
> service_dirs, so I think we're preserving order?

Er, thinko. My bad.

> > > +          /* Failing to set these up isn't fatal */
> > > +          _dbus_warn ("Unable to set up transient service directory: %s",
> > 
> > Nitpick: s/directory/directories/? Not sure what’s more appropriate here.
> > I’m fine with either, really.
> 
> There's currently up to 1. They'll only multiply if we decide we want some
> sort of hierarchy of transient directories (which systemd does have, because
> generators don't write to the same directories as ordinary transient
> services, but I think that's probably more complexity than we need).

Let’s stick with ‘directory’ then.
Comment 25 Philip Withnall 2017-02-15 16:09:37 UTC
(In reply to Simon McVittie from comment #22)
> (In reply to Philip Withnall from comment #18)
> > Nitpick: Add a trailing comma to eliminate future diff noise.
> 
> That isn't ISO C89, and as far as I'm aware dbus is still stuck in the 80s
> for Microsoft Visual C++'s benefit. I mitigated this by putting the
> oldest/most basic one last, so we can insert new entries above it.

Bother. I keep forgetting what the differences are between C89 and C99. :-(
Comment 26 Philip Withnall 2017-02-15 16:12:34 UTC
Comment on attachment 129632 [details] [review]
tests: Wrap file-deletion functions to handle EINTR

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

::: test/test-utils-glib.c
@@ +489,5 @@
> +test_remove_if_exists (const gchar *path)
> +{
> +  while (g_remove (path) != 0)
> +    {
> +      int saved_errno = errno;

What’s the purpose in saved_errno? As far as I can see, errno itself can’t change between the g_remove() call and the end of the loop iteration; and saved_errno is overwritten on each loop iteration.

@@ +510,5 @@
> +test_rmdir_must_exist (const gchar *path)
> +{
> +  while (g_remove (path) != 0)
> +    {
> +      int saved_errno = errno;

Same.
Comment 27 Philip Withnall 2017-02-15 16:15:32 UTC
Comment on attachment 129633 [details] [review]
test-utils-glib: Wait for the killed process to exit

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

++
Comment 28 Philip Withnall 2017-02-15 16:17:07 UTC
Comment on attachment 129634 [details] [review]
test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere  harmless

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

++
Comment 29 Philip Withnall 2017-02-15 16:21:21 UTC
Comment on attachment 129635 [details] [review]
sd-activation test: Create and destroy a temporary  XDG_RUNTIME_DIR

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

++

::: test/test-utils-glib.c
@@ +548,5 @@
> +test_rmdir_if_exists (const gchar *path)
> +{
> +  while (g_remove (path) != 0)
> +    {
> +      int saved_errno = errno;

Same comment as with the previous test-utils-glib.c patch: what’s the purpose in saved_errno?
Comment 30 Simon McVittie 2017-02-15 16:27:48 UTC
(In reply to Philip Withnall from comment #26)
> What’s the purpose in saved_errno? As far as I can see, errno itself can’t
> change between the g_remove() call and the end of the loop iteration; and
> saved_errno is overwritten on each loop iteration.

The purpose is that if I always use the saved_errno pattern, I don't have to reason about whether each individual use would be safe without it or not. I think your reasoning is correct here, but adding a _dbus_verbose() nearby would invalidate it (Bug #83625 is relevant).

errno is a sufficiently "sharp edge" that I prefer to adopt an unnecessarily cautious coding style around it.
Comment 31 Philip Withnall 2017-02-15 16:41:37 UTC
Comment on attachment 129636 [details] [review]
sd-activation test: Exercise transient services

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

A few comments/questions.

::: test/sd-activation.c
@@ +71,5 @@
>  
> +typedef enum
> +{
> +  FLAG_EARLY_TRANSIENT_SERVICE = (1 << 0),
> +  FLAG_NONE = 0

Nitpick: Would be a little more conventional to put FLAG_NONE first in the list. Unless you did this deliberately to avoid diff noise when adding more entries?

@@ +843,5 @@
> + * configuration.
> + */
> +static void
> +test_transient_services (Fixture *f,
> +    gconstpointer context G_GNUC_UNUSED)

That’s a lie. It’s not actually unused!

@@ +876,5 @@
> +
> +      dbus_message_unref (m);
> +      m = NULL;
> +
> +      /* It fails. */

Choose your own D-Bus testing adventure? :-)

(No changes needed here.)

@@ +900,5 @@
> +      fixture_create_transient_service (f, config->bus_name);
> +
> +      /* To guarantee that the transient service has been picked up, we have
> +       * to reload. */
> +

Nitpick: Seemingly unnecessary blank line.

::: test/test-utils-glib.c
@@ +571,5 @@
> +            gint mode)
> +{
> +  while (g_mkdir (path, mode) != 0)
> +    {
> +      int saved_errno = errno;

Same question about saved_errno as in previous reviews.
Comment 32 Philip Withnall 2017-02-15 16:43:41 UTC
(In reply to Simon McVittie from comment #30)
> (In reply to Philip Withnall from comment #26)
> > What’s the purpose in saved_errno? As far as I can see, errno itself can’t
> > change between the g_remove() call and the end of the loop iteration; and
> > saved_errno is overwritten on each loop iteration.
> 
> The purpose is that if I always use the saved_errno pattern, I don't have to
> reason about whether each individual use would be safe without it or not. I
> think your reasoning is correct here, but adding a _dbus_verbose() nearby
> would invalidate it (Bug #83625 is relevant).
> 
> errno is a sufficiently "sharp edge" that I prefer to adopt an unnecessarily
> cautious coding style around it.

Makes sense. Disregard all my review comments about saved_errno then. Instead, note that the g_strerror() calls you made were g_strerror(errno) rather than g_strerror(saved_errno).
Comment 33 Simon McVittie 2017-02-17 12:02:46 UTC
(In reply to Philip Withnall from comment #32)
> Makes sense. Disregard all my review comments about saved_errno then.
> Instead, note that the g_strerror() calls you made were g_strerror(errno)
> rather than g_strerror(saved_errno).

Good catch! I'll fix that.
Comment 34 Simon McVittie 2017-02-17 12:07:34 UTC
(In reply to Philip Withnall from comment #31)
> Nitpick: Would be a little more conventional to put FLAG_NONE first in the
> list. Unless you did this deliberately to avoid diff noise when adding more
> entries?

You guessed correctly; I'm putting the only item that every flags-set should have as the last one, to minimize diffstat noise.

> That’s a lie. It’s not actually unused!

I should fix that.

> Choose your own D-Bus testing adventure?

If you implement the method, return DBUS_HANDLER_RESULT_HANDLED, lose 1 luck point and turn to page 325. :-)
Comment 35 Simon McVittie 2017-02-17 12:18:50 UTC
Comment on attachment 129626 [details] [review]
config-parser: Eliminate duplicate functionality

Pushed, thanks for reviewing
Comment 36 Simon McVittie 2017-02-17 12:19:55 UTC
Comment on attachment 129627 [details] [review]
config-parser: Fix indentation

Pushed, with the same indentation fix applied to the corresponding 2 lines for system services (I'm going to have to edit both later in the branch).
Comment 37 Simon McVittie 2017-02-17 19:03:27 UTC
Created attachment 129700 [details] [review]
sd-activation test: Use a struct for the test context

This is going to be necessary to be able to influence setup() and
teardown() as well as just the individual tests.

---

Pulled earlier in the branch. No changes. Philip, I assume your objection on the basis of no trailing commas is not a blocker, given we're still stuck in C89 land?
Comment 38 Simon McVittie 2017-02-17 19:04:30 UTC
Created attachment 129701 [details] [review]
activation: Put activation directories in an ordered  list

There are two circumstances in which we load .service files. The first
is bus_activation_reload(), which is given an ordered list of directory
paths, and reads each one in its correct order, highest-precedence
first (normally ~/.local/share > /usr/local/share > /usr/share). This
seems correct.

However, if we are asked to activate a service for which we do not know
of a .service file, we opportunistically reload the search path and
try again, in the hope that it was recently-installed and not yet
discovered by inotify. Prior to this commit, this would iterate through
the hash table in arbitrary hash order, so we might load a service
from /usr/share even though it was meant to be masked by a
higher-priority service file in ~/.local/share or /usr/local/share.

Before I add more elements to the search path, we should make sure
it is always searched in the expected order.

We do not actually make use of the hash table's faster-than-O(n)
lookup by directory path anywhere, so there is no point in using a
hash table, and we can safely replace it with an ordered data structure.

---

New. This fixes a bug that has existed for quite a while, although perhaps it was never practically significant because inotify masked it for us.
Comment 39 Simon McVittie 2017-02-17 19:05:07 UTC
Created attachment 129702 [details] [review]
activation test: Use more realistic bus names for  services

---

New. Let's not rely on the dbus-daemon quietly accepting syntactically invalid bus names :-(
Comment 40 Simon McVittie 2017-02-17 19:05:59 UTC
Created attachment 129703 [details] [review]
test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as  an argument

---

Fixed memory leak on early-return code paths, as pointed out  in Philip's review.
Comment 41 Simon McVittie 2017-02-17 19:06:28 UTC
Created attachment 129704 [details] [review]
tests: Wrap file-deletion functions to handle EINTR

The GLib functions we're using don't, and it seems to be possible to be
interrupted during cleanup for our tests.

---

Use saved_errno more consistently
Comment 42 Simon McVittie 2017-02-17 19:07:48 UTC
Created attachment 129705 [details] [review]
test-utils-glib: Wait for the killed process to exit

---

Already r+ by Philip. No changes (I don't think), but should not be rebased before Attachment #129704 [details] to avoid SIGCHLD causing EINTR and breaking our tests.
Comment 43 Simon McVittie 2017-02-17 19:08:28 UTC
Created attachment 129706 [details] [review]
test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere  harmless

We want to be able to use <standard_system_datadirs/> in tests
without picking up someone else's .service files.

---

Unchanged, r+ by Philip
Comment 44 Simon McVittie 2017-02-17 19:09:00 UTC
Created attachment 129707 [details] [review]
config-parser: Remove dead code from system service  dirs test

progs was never used, because it was originally only used on
Windows, where this test makes no sense and so is no longer run.

It is unnecessary to check that the system service directories end
with dbus-1/system-services, because we are going to check their
exact values a short time later anyway.

It is also unnecessary to set XDG_DATA_HOME and XDG_DATA_DIRS,
because those variables are no longer respected for system service
directories, only for session service directories.

---

New.
Comment 45 Simon McVittie 2017-02-17 19:09:49 UTC
Created attachment 129708 [details] [review]
config-parser: Simplify test for standard session  service dirs

There's little point in asserting that the defaults (without
setting XDG_DATA_HOME, etc.) end with share/dbus-1/services,
because we are about to re-test with known values for XDG_DATA_HOME
etc., at which point we can check exact values which is more strict.

---

New.
Comment 46 Simon McVittie 2017-02-17 19:10:28 UTC
Created attachment 129709 [details] [review]
config-parser: Don't use dbus_setenv() to test service  directories

We can rely on the Autotools build system to pass in some safe values
for XDG_DATA_HOME and XDG_DATA_DIRS that match DBUS_TEST_BUILDDIR.

This test will now be skipped when running test-bus manually,
or under the CMake build system. Under CMake it could be reinstated
by setting the right environment variables.

---

New.
Comment 47 Simon McVittie 2017-02-17 19:11:11 UTC
Created attachment 129710 [details] [review]
config-parser test: Exercise the full config-parser

Don't just exercise _dbus_get_standard_session_servicedirs(), but
also its integration into the BusConfigParser.

---

New.
Comment 48 Simon McVittie 2017-02-17 19:12:17 UTC
Created attachment 129711 [details] [review]
sd-activation test: Create and destroy a temporary  XDG_RUNTIME_DIR

---

Now with better use of saved_errno.
Comment 49 Simon McVittie 2017-02-17 19:13:09 UTC
Created attachment 129712 [details] [review]
bus_config_parser_get_watched_dirs: Turn into a helper  function

This means we can test it more easily. At the moment it just
contains service directories, because this config file is so
cut-down that it doesn't have any config.d directories.

---

New.
Comment 50 Simon McVittie 2017-02-17 19:13:49 UTC
Created attachment 129713 [details] [review]
activation-helper: Rename  bus_config_parser_get_service_dirs

I'm about to change the version in the full-fat parser to return
BusServiceDir structs. Name this one with "paths" instead, to avoid
confusion.

---

New.
Comment 51 Simon McVittie 2017-02-17 19:15:11 UTC
Created attachment 129714 [details] [review]
config-parser: Store service directories in structs

This lets us give them a flags word, which we immediately use to
track whether this directory should be watched with inotify or
equivalent.

The struct name is unfortunately a bit odd, because I had aimed to
use BusServiceDir, but activation.c already has BusServiceDirectory
so that would have been too confusing.

---

New. This is why I wanted better test coverage for this stuff.
Comment 52 Simon McVittie 2017-02-17 19:19:00 UTC
Created attachment 129715 [details] [review]
activation: Add support for enforcing strict naming on  .service files

This is done on a per-directory basis.

The use of the BusContext here means we have to make the activation
test a little more realistic, by providing a non-NULL BusContext.

---

Specifically, I want transient service directories to enforce this.

A later refinement of this would be to make sure we prefer service files with the right name over service files with the wrong name, if we see both in the same directory, but I'm not sure how best to do that. We could scan the directory twice, or we could add code to supersede existing entries with better ones, but either way it'll take some refactoring.
Comment 53 Simon McVittie 2017-02-17 19:21:37 UTC
Created attachment 129716 [details] [review]
sysdeps: Add accessor for a list of transient service  directories

These directories can be used by service managers like `systemd --user`
and its generators, or by session infrastructure like gnome-session,
to synthesize D-Bus service files at runtime from some more canonical
source of information.

The intention is that this is in the XDG_RUNTIME_DIR as defined by the
freedesktop.org Base Directory Specification, which is private to the
user, and has a lifetime equal to the union of all the user's concurrent
login sessions.

This directory is provided on Linux systems that have systemd-logind and
pam_systemd, on other systems with PAM that have pam-xdg-support (which
has been abandoned by Ubuntu in favour of logind, but could be forked
by non-systemd environments that are interested in this functionality),
or any compatible reimplementation.

In practice this is most likely to be useful on systems that run
`dbus-daemon --session` from `systemd --user`.

---

Longer commit message explaining what's going on. Added more _dbus_verbose() as Philip suggested. Also rebased onto Bug #99828 having been fixed.
Comment 54 Simon McVittie 2017-02-17 19:22:56 UTC
Created attachment 129717 [details] [review]
config-parser: Add transient service directories

For configuration purposes these are treated as part of the standard
session service directories, to avoid having to add new configuration
syntax which would prevent an old dbus-daemon from reloading
successfully. From an API perspective, they're separate, though.

---

Now with immediate test coverage, and the new NO_WATCH and STRICT_NAMING flags.
Comment 55 Simon McVittie 2017-02-17 19:24:56 UTC
Created attachment 129718 [details] [review]
sd-activation test: Exercise transient services

To do this, we have to use the <standard_session_servicedirs/>.
A previous commit ensured that those don't provide any service files
we don't expect.

---

Now with saved_errno used more consistently as per Philip's review.

End of patch series.
Comment 56 Simon McVittie 2017-02-17 19:39:19 UTC
Created attachment 129719 [details] [review]
test-utils-glib: Windows: add errno.h for ENOENT, skip EINTR  checks

Windows apparently has and uses ENOENT for _unlink(), so this
should be safe enough.

EINTR is very much a POSIX thing, so ignore that on Windows.

---

Sigh, here's one more commit. With this, it builds on mingw-w64. We only actually call these functions on Unix at the moment, but they might be useful in portable tests in future.

This should ideally be squashed into (variously) Attachment #129704 [details], Attachment #129711 [details] and Attachment #129718 [details], but life's too short.
Comment 57 Simon McVittie 2017-02-17 19:47:25 UTC
(In reply to Simon McVittie from comment #1)
> Design - "nice to have"

These points have now been implemented. While implementing them, I found Bug #99828 and the bug fixed in Attachment #129701 [details], both of which have been around since the same commit shortly before dbus 0.21.
Comment 58 Simon McVittie 2017-02-17 20:58:31 UTC
Created attachment 129721 [details] [review]
spec: Don't say implementation-specific locations must be  lowest priority

We're treating transient services as higher-priority than those in
the XDG_DATA_HOME or XDG_DATA_DIRS, which is consistent with systemd.

---

For completeness, we even could consider expanding the search path to

XDG_CONFIG_HOME/dbus-1/services (user configuration)
XDG_CONFIG_DIRS/dbus-1/services (sysadmin configuration)
XDG_RUNTIME_DIR/dbus-1/services (transient)
XDG_DATA_HOME/dbus-1/services (user-installed software)
XDG_DATA_DIRS/dbus-1/services (Flatpak etc., then sysadmin-installed software)
${datadir}/dbus-1/services (software accompanying a custom dbus-daemon)

but the CONFIG path entries are not very useful until/unless we implement systemd-style masking (empty file or symlink to /dev/null "masks" files of the same name in lower-precedence directories).
Comment 59 Simon McVittie 2017-02-17 20:59:11 UTC
Created attachment 129722 [details] [review]
dbus-daemon(1): Describe how session services are found

---

For now I'm considering transient services to be an implementation-specific feature of the reference implementation, rather than part of the standard.
Comment 60 Philip Withnall 2017-02-18 23:33:54 UTC
Comment on attachment 129700 [details] [review]
sd-activation test: Use a struct for the test context

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

++

No objection to the lack of trailing comma.
Comment 61 Philip Withnall 2017-02-18 23:38:32 UTC
Comment on attachment 129701 [details] [review]
activation: Put activation directories in an ordered  list

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

This does mean we lose the behaviour that the hash table squashed duplicate directories, but:
 a) that’s not a functional problem — it just means we might check a given directory more than once; and
 b) the directories come straight from the configuration file, so the user would have had to list one twice.

++
Comment 62 Philip Withnall 2017-02-18 23:38:54 UTC
Comment on attachment 129702 [details] [review]
activation test: Use more realistic bus names for  services

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

++
Comment 63 Philip Withnall 2017-02-18 23:39:36 UTC
Comment on attachment 129703 [details] [review]
test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as  an argument

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

++
Comment 64 Philip Withnall 2017-02-18 23:40:13 UTC
Comment on attachment 129704 [details] [review]
tests: Wrap file-deletion functions to handle EINTR

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

++
Comment 65 Philip Withnall 2017-02-18 23:40:36 UTC
Comment on attachment 129705 [details] [review]
test-utils-glib: Wait for the killed process to exit

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

++ again for good measure.
Comment 66 Philip Withnall 2017-02-18 23:40:59 UTC
Comment on attachment 129706 [details] [review]
test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere  harmless

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

++ again for good measure.
Comment 67 Philip Withnall 2017-02-18 23:42:16 UTC
Comment on attachment 129707 [details] [review]
config-parser: Remove dead code from system service  dirs test

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

++
Comment 68 Philip Withnall 2017-02-18 23:43:35 UTC
Comment on attachment 129708 [details] [review]
config-parser: Simplify test for standard session  service dirs

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

++
Comment 69 Philip Withnall 2017-02-18 23:52:33 UTC
Comment on attachment 129709 [details] [review]
config-parser: Don't use dbus_setenv() to test service  directories

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

A few nitpicks/questions.

::: bus/config-parser.c
@@ +3452,5 @@
> +  if (dbus_test_builddir == NULL || xdg_data_home == NULL)
> +    {
> +      printf ("Not testing default session service directories because a "
> +              "build-time testing environment variable is not set: "
> +              "see AM_TESTS_ENVIRONMENT in tests/Makefile.am");

Nitpick: printf() call is missing the trailing \n.

::: test/Makefile.am
@@ +246,4 @@
>  	export XDG_RUNTIME_DIR=@abs_top_builddir@/test/XDG_RUNTIME_DIR; \
>  	export DBUS_FATAL_WARNINGS=1; \
> +	export DBUS_TEST_BUILDDIR=@abs_top_builddir@; \
> +	export DBUS_TEST_SRCDIR=@abs_top_srcdir@; \

Note that this is not exactly analogous to how GLib defines G_TEST_BUILDDIR and G_TEST_SRCDIR: they are defined using $builddir and $srcdir, not $top_builddir or $top_srcdir. So in this case they would end in `/test`.

That’s not necessarily a problem (this patch works), but it does introduce a bit of an incongruity. If you’re happy with that, that’s fine.
Comment 70 Philip Withnall 2017-02-19 00:02:35 UTC
Comment on attachment 129710 [details] [review]
config-parser test: Exercise the full config-parser

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

++, with this minor rebasing nitpick.

::: bus/config-parser.c
@@ +3472,4 @@
>      {
>        printf ("Not testing default session service directories because a "
>                "build-time testing environment variable is not set: "
> +              "see AM_TESTS_ENVIRONMENT in tests/Makefile.am\n");

Nitpick: As noted in the review for the previous patch, this should ideally be in that patch rather than this one.
Comment 71 Philip Withnall 2017-02-19 00:03:30 UTC
Comment on attachment 129711 [details] [review]
sd-activation test: Create and destroy a temporary  XDG_RUNTIME_DIR

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

++
Comment 72 Philip Withnall 2017-02-19 00:09:52 UTC
Comment on attachment 129712 [details] [review]
bus_config_parser_get_watched_dirs: Turn into a helper  function

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

A few nitpicks/questions, but the patch looks fundamentally sound.

::: bus/config-parser.c
@@ +2794,5 @@
> + *
> + * The list must be empty on entry. On success, the links are owned by the
> + * caller and must be freed, but the data in each link remains owned by
> + * the BusConfigParser and must not be freed: in GObject-Introspection
> + * notation, it is (transfer container).

Nitpick: Would be useful to mention that the list elements are strings (in GIR: (element-type utf8), even though technically they might not be UTF-8). Should probably also mention that the order is arbitrary, and that it’s possible for the returned list to be empty.

@@ +3580,5 @@
> +  for (link = _dbus_list_get_first_link (&watched_dirs), i = 0;
> +       link != NULL;
> +       link = _dbus_list_get_next_link (&watched_dirs, link), i++)
> +    {
> +      printf ("    watched service dir: '%s'\n", (char *) link->data);

(const char *) for the cast (since you’re not going to modify it)?

And below.

@@ +3584,5 @@
> +      printf ("    watched service dir: '%s'\n", (char *) link->data);
> +      printf ("    current standard service dir: '%s'\n",
> +              test_session_service_dir_matches[i]);
> +
> +      if (test_session_service_dir_matches[i] == NULL)

Do you also want to check that there are not more directories in the match set than are parsed?

(You might be doing this somewhere already, or it might not be relevant; sorry, I’m losing track of the patch set now.)
Comment 73 Philip Withnall 2017-02-19 00:11:00 UTC
Comment on attachment 129713 [details] [review]
activation-helper: Rename  bus_config_parser_get_service_dirs

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

++, bearing in mind this suggestion.

::: bus/config-parser-trivial.c
@@ +368,4 @@
>  }
>  
>  DBusList**
> +bus_config_parser_get_service_paths (BusConfigParser *parser)

Might be useful to add a short documentation comment to the tune of (element-type utf8).
Comment 74 Philip Withnall 2017-02-19 00:21:26 UTC
Comment on attachment 129714 [details] [review]
config-parser: Store service directories in structs

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

One question.

::: bus/activation.c
@@ +62,4 @@
>  {
>    int refcount;
>    char *dir_c;
> +  BusServiceDirFlags flags;

What’s this used for? It seems to be assigned to but never read from?
Comment 75 Philip Withnall 2017-02-19 08:59:50 UTC
Comment on attachment 129715 [details] [review]
activation: Add support for enforcing strict naming on  .service files

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

Looks fine, with one minor comment which you can fix or ignore as appropriate.

::: bus/activation.c
@@ +446,5 @@
> +           * still work on systemd systems, where we tell systemd to start the
> +           * SystemdService instead of launching dbus-daemon-launch-helper
> +           * ourselves. */
> +        }
> +      /* We could maybe log mismatched names for session services too,

Maybe make this comment a FIXME so it’s easier to find and come back to in future? Or file a follow-up bug and reference it in the comment.
Comment 76 Philip Withnall 2017-02-19 09:15:54 UTC
Comment on attachment 129716 [details] [review]
sysdeps: Add accessor for a list of transient service  directories

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

Minor leaks.

::: dbus/dbus-sysdeps-util-unix.c
@@ +1189,5 @@
>    return TRUE;
>  }
>  
> +static dbus_bool_t
> +ensure_owned_directory (const char *label,

`label` is unused. Is that intentional?

@@ +1303,5 @@
> +  if (xdg_runtime_dir == NULL)
> +    {
> +      _dbus_verbose ("XDG_RUNTIME_DIR is unset: transient session services "
> +                     "not available here\n");
> +      return TRUE;

This path leaks dbus1, services and xrd. Move it to the top of the function? Or use `ret = TRUE; goto out`?
Comment 77 Philip Withnall 2017-02-19 09:18:50 UTC
Comment on attachment 129717 [details] [review]
config-parser: Add transient service directories

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

++, nice.
Comment 78 Philip Withnall 2017-02-19 10:22:35 UTC
Comment on attachment 129718 [details] [review]
sd-activation test: Exercise transient services

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

++ with some nitpicks, if you want to fix them.

::: test/sd-activation.c
@@ +251,2 @@
>  setup (Fixture *f,
> +       gconstpointer context)

Nitpick: Indentation for the `context` argument has changed here. Which one is correct?

@@ +834,5 @@
>  
> +/*
> + * Test that we can set up transient services.
> + *
> + * If flags & FLAG_EARLY_TRANSIENT_SERVICE, we assert that a service that

Nitpick: I’d normally whack some brackets around a condition like (flags & FLAG_EARLY_TRANSIENT_SERVICE) to make it a bit easier for someone to parse the comment.
Comment 79 Philip Withnall 2017-02-19 10:23:40 UTC
Comment on attachment 129719 [details] [review]
test-utils-glib: Windows: add errno.h for ENOENT, skip EINTR  checks

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

++, life is indeed too short to spend it squashing commits like this.
Comment 80 Philip Withnall 2017-02-19 10:26:46 UTC
Comment on attachment 129721 [details] [review]
spec: Don't say implementation-specific locations must be  lowest priority

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

++

::: doc/dbus-specification.xml
@@ -5542,5 @@
> -            should be searched with lower priority than anything in
> -            XDG_DATA_HOME, XDG_DATA_DIRS or their respective defaults;
> -            for example, the reference implementation also
> -            looks in <literal>${datadir}/dbus-1/services</literal> as
> -            set at compile time.

Might it be good to still keep the “for example…” bit? Or do you want to move that to the dbus-daemon documentation because this is a specification, not a manual? I’m fine with either, just as long as this bit of information doesn’t get dropped entirely.
Comment 81 Philip Withnall 2017-02-19 10:32:48 UTC
Comment on attachment 129722 [details] [review]
dbus-daemon(1): Describe how session services are found

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

A few comments.

::: doc/dbus-daemon.1.xml.in
@@ +484,5 @@
>  
>  </itemizedlist>
>  
> +<para>&lt;standard_session_servicedirs/&gt; requests a standard set of
> +  session service directories. Its effect is similar to specifying a series

‘similar to’, or ‘equivalent to’?

@@ +496,5 @@
> +        <emphasis>$XDG_RUNTIME_DIR</emphasis>/dbus-1/services,
> +        if XDG_RUNTIME_DIR is set (see the XDG Base Directory
> +        Specification for details of XDG_RUNTIME_DIR):
> +        this location is suitable for transient services created at runtime
> +        by systemd generators, session managers or other session

Adding a context link for ‘systemd generators’ would be useful.

@@ +520,5 @@
> +    </listitem>
> +    <listitem>
> +      <para>
> +        the dbus-1/services subdirectory of the ${datadir} that
> +        was specified when dbus was compiled

Might want to restructure this so it’s more like the other bullet points, in that the full directory is listed first. Maybe:

<para>
  <emphasis>${datadir}</emphasis>/dbus-1/services, where ${datadir} was specified when dbus-daemon was compiled, and is typically /usr/share

@@ +544,5 @@
> +    <listitem>
> +      <para>
> +        A share/dbus-1/services directory found in the same
> +        directory hierarchy (prefix) as the dbus-daemon: this location
> +        is suitable for software stacks installed alongside dbus

Nitpick: s/alongside dbus/alongside dbus-daemon/?
Comment 82 Simon McVittie 2017-02-20 11:41:30 UTC
(In reply to Philip Withnall from comment #69)
> Nitpick: printf() call is missing the trailing \n.

Fixed in a subsequent patch. I'll rebase it into this one and push.

> Note that this is not exactly analogous to how GLib defines G_TEST_BUILDDIR
> and G_TEST_SRCDIR: they are defined using $builddir and $srcdir, not
> $top_builddir or $top_srcdir. So in this case they would end in `/test`.

I think I'm OK with that. Maybe one day we will have non-recursive make...

(In any case, these aren't G_TEST_BUILDDIR and G_TEST_SRCDIR, but our own new thing that is intentionally specific to D-Bus.)

(In reply to Philip Withnall from comment #72)
> Nitpick: Would be useful to mention that the list elements are strings (in
> GIR: (element-type utf8), even though technically they might not be UTF-8).
> Should probably also mention that the order is arbitrary, and that it’s
> possible for the returned list to be empty.

Makes sense. This seems minor enough that I'll probably just do it and push, rather than going back for another round of review.

> (const char *) for the cast (since you’re not going to modify it)?

This is copied from the existing code, but yes.

> Do you also want to check that there are not more directories in the match
> set than are parsed?

I'll check that we handle all failure cases (too many, too few, correct number but wrong content). Perhaps there's an extra line that I should have copied from the existing, similar test.

(In reply to Philip Withnall from comment #74)
> What’s this used for? It seems to be assigned to but never read from?

It's write-only (because the only flag added by this commit is irrelevant to activation.c) until we introduce BUS_SERVICE_DIR_FLAGS_STRICT_NAMING. Arguably I should move the addition of this struct member to the patch that adds STRICT_NAMING.

(In reply to Philip Withnall from comment #75)
> > +      /* We could maybe log mismatched names for session services too,
> 
> Maybe make this comment a FIXME so it’s easier to find and come back to in
> future? Or file a follow-up bug and reference it in the comment.

A follow-up bug would make sense.

(In reply to Philip Withnall from comment #76)
> `label` is unused. Is that intentional?

No. It was meant to be used in the error messages when we fail. I'll fix that.

> This path leaks dbus1, services and xrd. Move it to the top of the function?
> Or use `ret = TRUE; goto out`?

Makes sense, I'll fix that.

(In reply to Philip Withnall from comment #78)
> Nitpick: Indentation for the `context` argument has changed here. Which one
> is correct?

Either? Both? We don't really have a strong standard in dbus for whether to do "time-consuming indent" or just indent by 4 spaces.

> Nitpick: I’d normally whack some brackets around a condition like (flags &
> FLAG_EARLY_TRANSIENT_SERVICE) to make it a bit easier for someone to parse
> the comment.

Makes sense, will fix.

(In reply to Philip Withnall from comment #79)
> ++, life is indeed too short to spend it squashing commits like this.

Since I'm going to be changing some of the relevant commits anyway, I might squash this in.

(In reply to Philip Withnall from comment #80)
> Might it be good to still keep the “for example…” bit? Or do you want to
> move that to the dbus-daemon documentation because this is a specification,
> not a manual? I’m fine with either, just as long as this bit of information
> doesn’t get dropped entirely.

It's moving to the man page in the next commit.

(In reply to Philip Withnall from comment #81)
> > +<para>&lt;standard_session_servicedirs/&gt; requests a standard set of
> > +  session service directories. Its effect is similar to specifying a series
> 
> ‘similar to’, or ‘equivalent to’?

Technically not equivalent. There is currently no way to specify strict naming or lack of inotify for a <servicedir>. Perhaps there should be (but we wouldn't be able to use it until the dbus version that supports it is in widespread use, because that would break the ability for a running dbus-daemon to reload into new configuration).

> Adding a context link for ‘systemd generators’ would be useful.

This is a man page, so web links are not entirely useful, but systemd.generator(7) is relevant.

> Might want to restructure this so it’s more like the other bullet points, in
> that the full directory is listed first.

OK

> Nitpick: s/alongside dbus/alongside dbus-daemon/?

I suppose so. "Alongside all the software that was built from dbus.git", really, but dbus-daemon is the exemplar here.

(D-Bus is the protocol/specification, but dbus is the reference implementation of D-Bus.)
Comment 83 Philip Withnall 2017-02-20 13:26:32 UTC
(In reply to Simon McVittie from comment #82)
> (In reply to Philip Withnall from comment #69)
> > Note that this is not exactly analogous to how GLib defines G_TEST_BUILDDIR
> > and G_TEST_SRCDIR: they are defined using $builddir and $srcdir, not
> > $top_builddir or $top_srcdir. So in this case they would end in `/test`.
> 
> I think I'm OK with that. Maybe one day we will have non-recursive make...
> 
> (In any case, these aren't G_TEST_BUILDDIR and G_TEST_SRCDIR, but our own
> new thing that is intentionally specific to D-Bus.)

I know, but given the naming I was assuming you might want to keep the semantics the same as the GLib ones, so that people don’t have to remember how they differ. Whatever.

> (In reply to Philip Withnall from comment #74)
> > What’s this used for? It seems to be assigned to but never read from?
> 
> It's write-only (because the only flag added by this commit is irrelevant to
> activation.c) until we introduce BUS_SERVICE_DIR_FLAGS_STRICT_NAMING.
> Arguably I should move the addition of this struct member to the patch that
> adds STRICT_NAMING.

I guess moving it in the patch set would make things neater, but I don’t think it’s worth spending time on if it’s a hassle. It’s not doing any harm being write-only for a couple of commits.

> Either? Both? We don't really have a strong standard in dbus for whether to
> do "time-consuming indent" or just indent by 4 spaces.

Well, Linux is about choice. ¯\_(ツ)_/¯

> (In reply to Philip Withnall from comment #80)
> > Might it be good to still keep the “for example…” bit? Or do you want to
> > move that to the dbus-daemon documentation because this is a specification,
> > not a manual? I’m fine with either, just as long as this bit of information
> > doesn’t get dropped entirely.
> 
> It's moving to the man page in the next commit.

Great.

> (In reply to Philip Withnall from comment #81)
> > > +<para>&lt;standard_session_servicedirs/&gt; requests a standard set of
> > > +  session service directories. Its effect is similar to specifying a series
> > 
> > ‘similar to’, or ‘equivalent to’?
> 
> Technically not equivalent. There is currently no way to specify strict
> naming or lack of inotify for a <servicedir>. Perhaps there should be (but
> we wouldn't be able to use it until the dbus version that supports it is in
> widespread use, because that would break the ability for a running
> dbus-daemon to reload into new configuration).

It might be worth briefly mentioning the reasons for that non-equivalence, then. I don’t currently see a need to support making them equivalent though.

> > Adding a context link for ‘systemd generators’ would be useful.
> 
> This is a man page, so web links are not entirely useful, but
> systemd.generator(7) is relevant.

Great.

> > Nitpick: s/alongside dbus/alongside dbus-daemon/?
> 
> I suppose so. "Alongside all the software that was built from dbus.git",
> really, but dbus-daemon is the exemplar here.

‘dbus-daemon’ (not ‘dbus’) is what’s consistently used throughout the rest of the page.

> (D-Bus is the protocol/specification, but dbus is the reference
> implementation of D-Bus.)

I know. :-)
Comment 84 Simon McVittie 2017-02-20 17:19:06 UTC
(In reply to Philip Withnall from comment #83)
> I know, but given the naming I was assuming you might want to keep the
> semantics the same as the GLib ones, so that people don’t have to remember
> how they differ.

OK, I'm convinced. Changing this.
Comment 85 Simon McVittie 2017-02-20 17:25:01 UTC
(In reply to Philip Withnall from comment #69)
> Nitpick: printf() call is missing the trailing \n.

Fixed in https://github.com/smcv/dbus/commit/4be14aab914cb6ab9703b1aaba2f4c973a7b729c which I'll push to master if Travis-CI is happy with it.

> Note that this is not exactly analogous to how GLib defines G_TEST_BUILDDIR
> and G_TEST_SRCDIR: they are defined using $builddir and $srcdir, not
> $top_builddir or $top_srcdir. So in this case they would end in `/test`.

Also fixed in 4be14aab.

(In reply to Philip Withnall from comment #79)
> test-utils-glib: Windows: add errno.h for ENOENT, skip EINTR  checks

I rolled the first part of this into https://github.com/smcv/dbus/commit/898ae926dfa0346ad49ed652bd3004705dfc069d which I'll also push to master if Travis-CI is successful. The other parts are squashed into the other affected commits since I needed to alter them anyway.
Comment 86 Philip Withnall 2017-02-20 17:48:09 UTC
(In reply to Simon McVittie from comment #85)
> (In reply to Philip Withnall from comment #69)
> > Nitpick: printf() call is missing the trailing \n.
> 
> Fixed in
> https://github.com/smcv/dbus/commit/4be14aab914cb6ab9703b1aaba2f4c973a7b729c
> which I'll push to master if Travis-CI is happy with it.

LGTM.

> > Note that this is not exactly analogous to how GLib defines G_TEST_BUILDDIR
> > and G_TEST_SRCDIR: they are defined using $builddir and $srcdir, not
> > $top_builddir or $top_srcdir. So in this case they would end in `/test`.
> 
> Also fixed in 4be14aab.

LGTM.

> (In reply to Philip Withnall from comment #79)
> > test-utils-glib: Windows: add errno.h for ENOENT, skip EINTR  checks
> 
> I rolled the first part of this into
> https://github.com/smcv/dbus/commit/898ae926dfa0346ad49ed652bd3004705dfc069d
> which I'll also push to master if Travis-CI is successful. The other parts
> are squashed into the other affected commits since I needed to alter them
> anyway.

LGTM.
Comment 87 Simon McVittie 2017-02-20 18:18:23 UTC
(In reply to Simon McVittie from comment #42)
> test-utils-glib: Wait for the killed process to exit

Turns out running this on Travis-CI (which runs make distcheck) exposes a pre-existing bug: we try to kill pid 0 if some tests are skipped. I'll attach a fix when I've worked through all the distchecking to get that far through the branch.
Comment 88 Simon McVittie 2017-02-20 18:39:50 UTC
Comment on attachment 129700 [details] [review]
sd-activation test: Use a struct for the test context

pushed as 84c8403197c95272a7112eb1fddb84ad2b7474be
Comment 89 Simon McVittie 2017-02-20 18:40:10 UTC
Comment on attachment 129701 [details] [review]
activation: Put activation directories in an ordered  list

Pushed as 6980c17220da2446614f55f7898edbf5bf2af65a
Comment 90 Simon McVittie 2017-02-20 18:40:27 UTC
Comment on attachment 129702 [details] [review]
activation test: Use more realistic bus names for  services

Pushed as 96e6b3698ddda314066870e7faa263f129436581
Comment 91 Simon McVittie 2017-02-20 18:40:44 UTC
Comment on attachment 129703 [details] [review]
test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as  an argument

Pushed as eef176eb72c17773610ef2780133ad2a65fd26c2
Comment 92 Simon McVittie 2017-02-20 18:41:07 UTC
Comment on attachment 129704 [details] [review]
tests: Wrap file-deletion functions to handle EINTR

Pushed as 898ae926dfa0346ad49ed652bd3004705dfc069d, incorporating later Windows fixes
Comment 93 Simon McVittie 2017-02-20 18:41:37 UTC
Comment on attachment 129706 [details] [review]
test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere  harmless

Pushed as 867a600bf7f8716241e5aa1cf1024ce113182198, rebased ahead of the commit that broke distcheck
Comment 94 Simon McVittie 2017-02-20 18:42:05 UTC
Comment on attachment 129707 [details] [review]
config-parser: Remove dead code from system service  dirs test

Pushed as 81c82d2f34b616cdc80a5a332fcb11aab85dded4, rebased ahead of the commit that broke distcheck
Comment 95 Simon McVittie 2017-02-20 18:50:40 UTC
Created attachment 129762 [details] [review]
[1] config-parser: Simplify test for standard session service  dirs

---

Already reviewed and will be pushed when travis-ci is happy
Comment 96 Simon McVittie 2017-02-20 18:51:37 UTC
Created attachment 129763 [details] [review]
[2] config-parser: Don't use dbus_setenv() to test service  directories

We can rely on the Autotools build system to pass in some safe values
for XDG_DATA_HOME and XDG_DATA_DIRS that match DBUS_TEST_BUILDDIR.

This test will now be skipped when running test-bus manually,
or under the CMake build system. Under CMake it could be reinstated
by setting the right environment variables.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99825
Reviewed-by: Philip Withnall <withnall@endlessm.com>
[smcv: add missing newline as requested]
[smcv: align DBUS_TEST_BUILDDIR with G_TEST_BUILDDIR]
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

---

Essentially already reviewed, probably no need to review again.
Comment 97 Simon McVittie 2017-02-20 18:52:05 UTC
Created attachment 129764 [details] [review]
[3] config-parser test: Exercise the full config-parser

---

Already reviewed
Comment 98 Simon McVittie 2017-02-20 18:52:30 UTC
Created attachment 129765 [details] [review]
[4] tests: Consistently don't try to kill pid 0

daemon_pid will still be 0 if any tests were skipped. In particular
this happens during `make installcheck`.

---

New.
Comment 99 Simon McVittie 2017-02-20 18:53:58 UTC
Created attachment 129766 [details] [review]
[5] test-utils-glib: Wait for the killed process to exit

Otherwise, removing transient service directories that are being
watched by the dbus-daemon can fail with EAGAIN.

---

Already reviewed, doesn't really need re-review, but it breaks distcheck unless we apply Attachment #129765 [details] first.
Comment 100 Simon McVittie 2017-02-20 18:54:44 UTC
Created attachment 129767 [details] [review]
[6] sd-activation test: Create and destroy a temporary  XDG_RUNTIME_DIR

---

Essentially already reviewed, I just incorporated part of Attachment #129719 [details] into it.
Comment 101 Simon McVittie 2017-02-20 18:56:28 UTC
Created attachment 129768 [details] [review]
[7] bus_config_parser_get_watched_dirs: Turn into a helper  function

---

Expand the doc-comment as requested. Use (const char *) casts instead of (char *) as requested. Copy a few extra lines from further up the test so we check for the case where the resulting list is a prefix of the list we expected.
Comment 102 Simon McVittie 2017-02-20 18:57:31 UTC
Created attachment 129769 [details] [review]
[8] activation-helper: Rename  bus_config_parser_get_service_dirs

---

Essentially already reviewed, no real need to do so again. I added a brief doc-comment in pseudo-Doxygen syntax (the parts of dbus that have API documentation use Doxygen).
Comment 103 Simon McVittie 2017-02-20 18:58:36 UTC
Created attachment 129770 [details] [review]
[9] config-parser: Store service directories in structs

---

Unused flags member not included in BusServiceDirectory here yet, I moved it to the next commit.
Comment 104 Simon McVittie 2017-02-20 19:01:55 UTC
Created attachment 129771 [details] [review]
[10] activation: Add support for enforcing strict naming on  .service files

This is done on a per-directory basis.

The use of the BusContext here means we have to make the activation
test a little more realistic, by providing a non-NULL BusContext.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99825
Reviewed-by: Philip Withnall <withnall@endlessm.com>
[smcv: move flags in BusServiceDirectory from previous commit to here]
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

---

Doesn't really need re-review unless you want to.
Comment 105 Simon McVittie 2017-02-20 19:03:28 UTC
(In reply to Simon McVittie from comment #104)
> [10] activation: Add support for enforcing strict naming on  .service files

Actually, ignore this one, I need to include bug references in it.
Comment 106 Simon McVittie 2017-02-20 19:09:07 UTC
Comment on attachment 129762 [details] [review]
[1] config-parser: Simplify test for standard session service  dirs

Pushed
Comment 107 Simon McVittie 2017-02-20 19:09:14 UTC
Comment on attachment 129763 [details] [review]
[2] config-parser: Don't use dbus_setenv() to test service  directories

Pushed
Comment 108 Simon McVittie 2017-02-20 19:09:21 UTC
Comment on attachment 129764 [details] [review]
[3] config-parser test: Exercise the full config-parser

Pushed
Comment 109 Simon McVittie 2017-02-20 19:33:45 UTC
Created attachment 129773 [details] [review]
[10] activation: Add support for enforcing strict naming on  .service files

---

Add bug references. Move addition of BusServiceDirFlags to BusServiceDirectory from previous commit to here. Log a _dbus_verbose() for Bug #99873 for now.
Comment 110 Simon McVittie 2017-02-20 19:35:11 UTC
Created attachment 129774 [details] [review]
[11] sysdeps: Add accessor for a list of transient service  directories

---

Use label in ensure_owned_directory() as intended. "goto out" to fix memory leak if XDG_RUNTIME_DIR is unset.
Comment 111 Simon McVittie 2017-02-20 19:36:43 UTC
Created attachment 129775 [details] [review]
[12] config-parser: Add transient service directories

---

To make this test work during distcheck, we have to ensure the directory exists; do so.
Comment 112 Simon McVittie 2017-02-20 19:38:01 UTC
Created attachment 129776 [details] [review]
[13] sd-activation test: Exercise transient services

---

Don't re-indent context argument. Add parentheses around an "&" operation in a comment. Incorporate the last part of Attachment #129719 [details].
Comment 113 Simon McVittie 2017-02-20 19:38:40 UTC
Created attachment 129777 [details] [review]
[14] spec: Don't say implementation-specific locations must  be lowest priority

We're treating transient services as higher-priority than those in
the XDG_DATA_HOME or XDG_DATA_DIRS, which is consistent with systemd.

The specific list used by the standard session dbus-daemon will be
added to dbus-daemon(1) in the next commit.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99825
Reviewed-by: Philip Withnall <withnall@endlessm.com>
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

---

Added a note to the commit message about the "for example" having moved to dbus-daemon(1).
Comment 114 Simon McVittie 2017-02-20 19:50:26 UTC
Created attachment 129778 [details] [review]
[15] dbus-daemon(1): Describe how session services are found

---

Explain how the transient directory is special. Consistently say dbus-daemon instead of dbus. Cross-reference systemd.generator(7).
Comment 115 Ralf Habacker 2017-02-20 21:58:13 UTC
Comment on attachment 129778 [details] [review]
[15] dbus-daemon(1): Describe how session services are found

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

::: doc/dbus-daemon.1.xml.in
@@ +561,5 @@
> +  On Windows, the standard session service directories are:
> +  <itemizedlist>
> +    <listitem>
> +      <para>
> +        <emphasis>%CommonProgramFiles%</emphasis>/dbus-1/services

[1]

@@ +569,5 @@
> +    </listitem>
> +    <listitem>
> +      <para>
> +        A share/dbus-1/services directory found in the same
> +        directory hierarchy (prefix) as the dbus-daemon: this location

[2]

@@ +575,5 @@
> +      </para>
> +    </listitem>
> +  </itemizedlist>
> +</para>
> +

Does the mentioned order of this directories indicates the priority from which services are picked ?
e.g if a service file providing a dedicated service is located in [1] and the same service is in [2] which one is used ?

There should be a related note to be clear about this.
Comment 116 Philip Withnall 2017-02-21 10:34:36 UTC
Comment on attachment 129765 [details] [review]
[4] tests: Consistently don't try to kill pid 0

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

++
Comment 117 Philip Withnall 2017-02-21 10:35:54 UTC
Comment on attachment 129767 [details] [review]
[6] sd-activation test: Create and destroy a temporary  XDG_RUNTIME_DIR

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

++
Comment 118 Philip Withnall 2017-02-21 10:37:12 UTC
Comment on attachment 129768 [details] [review]
[7] bus_config_parser_get_watched_dirs: Turn into a helper  function

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

++
Comment 119 Philip Withnall 2017-02-21 10:37:42 UTC
Comment on attachment 129769 [details] [review]
[8] activation-helper: Rename  bus_config_parser_get_service_dirs

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

++
Comment 120 Philip Withnall 2017-02-21 10:39:17 UTC
Comment on attachment 129770 [details] [review]
[9] config-parser: Store service directories in structs

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

++
Comment 121 Philip Withnall 2017-02-21 10:40:42 UTC
Comment on attachment 129773 [details] [review]
[10] activation: Add support for enforcing strict naming on  .service files

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

++
Comment 122 Philip Withnall 2017-02-21 10:43:01 UTC
Comment on attachment 129774 [details] [review]
[11] sysdeps: Add accessor for a list of transient service  directories

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

++
Comment 123 Philip Withnall 2017-02-21 10:44:22 UTC
Comment on attachment 129775 [details] [review]
[12] config-parser: Add transient service directories

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

++
Comment 124 Philip Withnall 2017-02-21 10:45:42 UTC
Comment on attachment 129776 [details] [review]
[13] sd-activation test: Exercise transient services

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

++
Comment 125 Philip Withnall 2017-02-21 10:46:19 UTC
Comment on attachment 129777 [details] [review]
[14] spec: Don't say implementation-specific locations must  be lowest priority

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

++
Comment 126 Philip Withnall 2017-02-21 10:48:33 UTC
Comment on attachment 129778 [details] [review]
[15] dbus-daemon(1): Describe how session services are found

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

Looks good to me, though Ralf has some comments above.
Comment 127 Simon McVittie 2017-02-21 11:55:12 UTC
Created attachment 129789 [details] [review]
dbus-daemon(1): Describe how session and system services are  found

For Unix, this partially duplicates the D-Bus Specification, but
provides more detail about the intention of each search path element.
It also documents the non-standardized path elements searched by the
reference implementation.

For Windows, there are no standardized path elements in the D-Bus
Specification (and it isn't clear how useful it would be to standardize
them, since Windows software that uses D-Bus tends to be installed
as an integrated "stack" with a bundled copy of a suitable dbus-daemon),
so we just document what the reference implementation does.

---

Explicitly say what the search order is - as Ralf pointed out, we weren't particularly clear about that.

Also say which search path entries are defined by the D-Bus Specification (and so should be searched by any implementation of a bus daemon) and which ones are specific to the reference dbus-daemon.

Also mention how XDG_DATA_DIRS can be extended by users and/or sysadmins, with Flatpak as a concrete example of something that wants to do so.

Also write similar text for system services.

Also stop saying service files are primarily for session services, which hasn't been true since system bus activation was implemented 9½ years ago.

I'm sure there is more missing or inaccurate information in the parts of this man page that I didn't edit here - but if there is, please propose additional patches rather than blocking this one :-)
Comment 128 Philip Withnall 2017-02-21 12:13:28 UTC
Comment on attachment 129789 [details] [review]
dbus-daemon(1): Describe how session and system services are  found

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

++ with a couple of nitpicks which you could fix if you really wanted to.

::: doc/dbus-daemon.1.xml.in
@@ +496,5 @@
> +  of &lt;servicedir/&gt; elements for each of the data directories,
> +  in the order given here.
> +  It is not exactly equivalent, because there is currently no way
> +  to disable directory monitoring or enforce strict service file naming
> +  for a &lt;servicedir/&gt;.</para>

Nitpick: The indentation of this <para> differs from the others, but that’s not a blocker.

@@ +580,5 @@
> +        <emphasis>${datadir}</emphasis>/dbus-1/services
> +        for the <emphasis>${datadir}</emphasis> that was specified when
> +        dbus was compiled, typically /usr/share: this location is an
> +        extension provided by the reference dbus-daemon implementation,
> +        and is suitable for software stacks installed alongside dbus-daemon

Nitpick: Missing final full stop.
Comment 129 Simon McVittie 2017-02-21 12:56:39 UTC
Created attachment 129791 [details] [review]
test: Create XDG_RUNTIME_DIR with restrictive permissions

Otherwise the test for transient services fails on Travis-CI,
where umask is 002 (new directories are 775, rwxrwxr-x).

---

Could be squashed into Attachment #129776 [details].
Comment 130 Simon McVittie 2017-02-21 12:57:19 UTC
Created attachment 129792 [details] [review]
sd-activation test: Don't declare unused variable with  old AppArmor

AppArmor 2.9.0 on Debian jessie doesn't have aa_features, and in
any case this variable is unused with the older AppArmor versions
that skip this test.

---

Could be squashed into Attachment #129776 [details] (I think that's the right one).
Comment 131 Simon McVittie 2017-02-21 13:00:12 UTC
(In reply to Simon McVittie from comment #129)
> Created attachment 129791 [details] [review]
> test: Create XDG_RUNTIME_DIR with restrictive permissions
> 
> Otherwise the test for transient services fails on Travis-CI,
> where umask is 002 (new directories are 775, rwxrwxr-x).
> 
> ---
> 
> Could be squashed into Attachment #129776 [details]

No, it could be squashed into Attachment #129775 [details].

(In reply to Simon McVittie from comment #130)
> sd-activation test: Don't declare unused variable with  old AppArmor
> 
> AppArmor 2.9.0 on Debian jessie doesn't have aa_features, and in
> any case this variable is unused with the older AppArmor versions
> that skip this test.
> 
> ---
> 
> Could be squashed into Attachment #129776 [details] (I think that's the
> right one).

No, it could be squashed into Attachment #129767 [details].

I hope that's everything...
Comment 132 Philip Withnall 2017-02-21 13:17:31 UTC
Comment on attachment 129791 [details] [review]
test: Create XDG_RUNTIME_DIR with restrictive permissions

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

++
Comment 133 Philip Withnall 2017-02-21 13:18:34 UTC
Comment on attachment 129792 [details] [review]
sd-activation test: Don't declare unused variable with  old AppArmor

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

++
Comment 134 Simon McVittie 2017-02-21 13:37:29 UTC
Fixed in git for 1.11.12. Thanks for all the reviews!
Comment 135 Simon McVittie 2017-02-21 17:20:18 UTC
Created attachment 129800 [details] [review]
Add a simple integration test for transient services

Because this is in a subdirectory, it requires some extra `mkdir -p`
and some `nobase_` variables. Make all the installed-tests `nobase_`
for consistency.

---

One more - let's have an integration test for system-level functionality.

As a GNOME-style installed-test, this is expected to be run with a desktop session already up; but because we want it to be as easy to run as possible, and because we require a systemd-style XDG_RUNTIME_DIR (and for part of the test also systemd itself), we skip the test if that session is either absent or not systemd.
Comment 136 Simon McVittie 2017-02-21 18:05:10 UTC
Created attachment 129804 [details] [review]
Add a simple integration test for transient services

Because this is in a subdirectory, it requires some extra `mkdir -p`
and some `nobase_` variables. Make all the installed-tests `nobase_`
for consistency.

---

Andre Moreira Magalhaes took a look at the previous version and pointed out that the use of ExecStartPre was rather odd. It was a leftover from trying (without success unfortunately) to persuade systemd to tell us that activation had failed.
Comment 137 Andre Moreira Magalhaes 2017-02-21 18:07:03 UTC
Comment on attachment 129804 [details] [review]
Add a simple integration test for transient services

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

Looks good to me.
Comment 138 Simon McVittie 2017-02-21 18:39:58 UTC
All applied. Fixed in git for 1.11.12.


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.