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.
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. Created attachment 129627 [details] [review] config-parser: Fix indentation Created attachment 129628 [details] [review] sysdeps: Add accessor for a list of transient service directories 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. Created attachment 129630 [details] [review] test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as an argument 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. 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. 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. 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. Created attachment 129635 [details] [review] sd-activation test: Create and destroy a temporary XDG_RUNTIME_DIR 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 on attachment 129626 [details] [review] config-parser: Eliminate duplicate functionality Review of attachment 129626 [details] [review]: ----------------------------------------------------------------- ++ 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 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 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 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 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. (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. (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). (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. (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. (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. (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. (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 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 on attachment 129633 [details] [review] test-utils-glib: Wait for the killed process to exit Review of attachment 129633 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129634 [details] [review] test: Redirect XDG_DATA_HOME, XDG_DATA_DIRS somewhere harmless Review of attachment 129634 [details] [review]: ----------------------------------------------------------------- ++ 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? (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 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. (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). (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. (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 on attachment 129626 [details] [review] config-parser: Eliminate duplicate functionality Pushed, thanks for reviewing 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). 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? 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. 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 :-( 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. 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 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. 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 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. 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. 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. 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. Created attachment 129711 [details] [review] sd-activation test: Create and destroy a temporary XDG_RUNTIME_DIR --- Now with better use of saved_errno. 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. 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. 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. 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. 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. 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. 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. 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. (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. 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). 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 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 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 on attachment 129702 [details] [review] activation test: Use more realistic bus names for services Review of attachment 129702 [details] [review]: ----------------------------------------------------------------- ++ 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 on attachment 129704 [details] [review] tests: Wrap file-deletion functions to handle EINTR Review of attachment 129704 [details] [review]: ----------------------------------------------------------------- ++ 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 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 on attachment 129707 [details] [review] config-parser: Remove dead code from system service dirs test Review of attachment 129707 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129708 [details] [review] config-parser: Simplify test for standard session service dirs Review of attachment 129708 [details] [review]: ----------------------------------------------------------------- ++ 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 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 on attachment 129711 [details] [review] sd-activation test: Create and destroy a temporary XDG_RUNTIME_DIR Review of attachment 129711 [details] [review]: ----------------------------------------------------------------- ++ 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 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 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 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 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 on attachment 129717 [details] [review] config-parser: Add transient service directories Review of attachment 129717 [details] [review]: ----------------------------------------------------------------- ++, nice. 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 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 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 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><standard_session_servicedirs/> 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/? (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><standard_session_servicedirs/> 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.) (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><standard_session_servicedirs/> 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. :-) (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. (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. (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. (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 on attachment 129700 [details] [review] sd-activation test: Use a struct for the test context pushed as 84c8403197c95272a7112eb1fddb84ad2b7474be Comment on attachment 129701 [details] [review] activation: Put activation directories in an ordered list Pushed as 6980c17220da2446614f55f7898edbf5bf2af65a Comment on attachment 129702 [details] [review] activation test: Use more realistic bus names for services Pushed as 96e6b3698ddda314066870e7faa263f129436581 Comment on attachment 129703 [details] [review] test_get_dbus_daemon: Take a custom XDG_RUNTIME_DIR as an argument Pushed as eef176eb72c17773610ef2780133ad2a65fd26c2 Comment on attachment 129704 [details] [review] tests: Wrap file-deletion functions to handle EINTR Pushed as 898ae926dfa0346ad49ed652bd3004705dfc069d, incorporating later Windows fixes 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 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 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 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. Created attachment 129764 [details] [review] [3] config-parser test: Exercise the full config-parser --- Already reviewed 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. 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. 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. 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. 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). 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. 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. (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 on attachment 129762 [details] [review] [1] config-parser: Simplify test for standard session service dirs Pushed Comment on attachment 129763 [details] [review] [2] config-parser: Don't use dbus_setenv() to test service directories Pushed Comment on attachment 129764 [details] [review] [3] config-parser test: Exercise the full config-parser Pushed 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. 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. 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. 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]. 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). 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 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 on attachment 129765 [details] [review] [4] tests: Consistently don't try to kill pid 0 Review of attachment 129765 [details] [review]: ----------------------------------------------------------------- ++ 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 on attachment 129768 [details] [review] [7] bus_config_parser_get_watched_dirs: Turn into a helper function Review of attachment 129768 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129769 [details] [review] [8] activation-helper: Rename bus_config_parser_get_service_dirs Review of attachment 129769 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129770 [details] [review] [9] config-parser: Store service directories in structs Review of attachment 129770 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129773 [details] [review] [10] activation: Add support for enforcing strict naming on .service files Review of attachment 129773 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129774 [details] [review] [11] sysdeps: Add accessor for a list of transient service directories Review of attachment 129774 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129775 [details] [review] [12] config-parser: Add transient service directories Review of attachment 129775 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129776 [details] [review] [13] sd-activation test: Exercise transient services Review of attachment 129776 [details] [review]: ----------------------------------------------------------------- ++ 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 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. 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 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 <servicedir/> 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 <servicedir/>.</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. 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]. 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). (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 on attachment 129791 [details] [review] test: Create XDG_RUNTIME_DIR with restrictive permissions Review of attachment 129791 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 129792 [details] [review] sd-activation test: Don't declare unused variable with old AppArmor Review of attachment 129792 [details] [review]: ----------------------------------------------------------------- ++ Fixed in git for 1.11.12. Thanks for all the reviews! 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. 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 on attachment 129804 [details] [review] Add a simple integration test for transient services Review of attachment 129804 [details] [review]: ----------------------------------------------------------------- Looks good to me. 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.
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.