Created attachment 113751 [details] [review]
[2/2] Do not fail dbus startup is system.d/session.d files are
 missing

(In reply to xnox from comment #0)
> * add support for "ignore_missing" attribute in the includedir element,
> similar to include element.

This seems reasonable, although I wonder whether it should just not be an option, and never fail on ENOENT. I can't think of any situations in which you'd want to distinguish between "drop-in directory is empty" and "drop-in directory doesn't exist at all" - can you?

> In my case, I simply use ./configure --sysconfdir=/usr/share to achieve the
> switch and ship all configuration in /usr/share/dbus-1/ rather than in /etc.
> And this works very well.

I don't like this. Configuring sysconfdir = /etc is The Right Thing, even if we want to allow it to be empty.

If we make dbus-daemon 1.y read ${datadir}/dbus-1/system.conf, then I think it should *always* read ${datadir}/dbus-1/system.conf (so we can document it sensibly), rather than being a per-distribution choice.

Here's a possible design sketch (I'm using the system bus as my example throughout, but the same applies equally to the session bus).

* dbus-daemon --system reads ${datadir}/dbus-1/system.conf,
  which we can guarantee exists in any non-faulty installation

* ${datadir}/dbus-1/system.conf is the current bus/system.conf.in,
  with variable substitutions applied, and with more includes:

  - does the semi-hard-coded setup (everything above the first
    <includedir/>)
  - then includes ${sysconfdir}/dbus-1/system.conf if it exists
  - then system.d/ as a directory if it exists
  - then ${sysconfdir}/dbus-1/system.d/ if it exists
  - then ${sysconfdir}/dbus-1/system-local.conf if it exists
  - then whatever this contexts/dbus_contexts thing is, in one or
    both paths (do SELinux distros actually ship this? is it
    sysadmin configuration or distro configuration or what?
    maybe Red Hat people would know...)

* We install a new bus/etc-dbus-1/system.conf.in (or something) which will
  land in /etc by default (so that the conffile mechanisms in Debian and its
  derivatives offer to replace the current one, handling user modifications
  correctly); it is functionally empty (I think in practice it needs
  a <busconfig></busconfig>), and has comments saying that it is
  deprecated, and you should create system.d/local.conf or system-local.conf
  if you want to adjust the bus config

* For regression tests, instead of copying bus/system.conf into the test
  data directory unmodified, we redo the substitutions, replacing
  ${sysconfdir} with the installed-test data directory
  ${testexecdir}/data, to avoid using the actual system copies here

We can eventually delete /etc/dbus-1/system.conf entirely, and embedded distros can do so if they want to, but I think it has existed for so long (~ 10 years) that replacing it with a comment saying "this is deprecated" is the more friendly approach.

AIUI, the difference between system.conf and system-local.conf is that RPM distributions forcibly replace the former during upgrades (there is never a dpkg-style conffile prompt in RPM-land), and never touch the latter: which, for those with a more Debian mindset, means system.conf should really have been in ${datadir}, and system-local.conf in ${sysconfdir}.

Rationale for that specific search path:

* Distro snippets in system.d have historically taken precedence over system.conf. They should continue to do so when moved to the datadir.

* Everything else being equal, /etc should generally take precedence over /usr.

* system-local.conf makes no sense in ${datadir}, because it is explicitly for the local sysadmin, not for the OS integrator.

* system-local.conf has the highest precedence, by design...

* ... but whatever this SELinux context thing is has traditionally had even higher precedence than system-local.conf.

If a sysadmin on a Debian derivative has modified /etc/dbus-1/system.conf, and keeps their modified version, then their dbus-daemon will redundantly parse /etc/dbus-1/system.d and /etc/dbus-1/system-local.conf twice - but I don't think that's the end of the world.

(In reply to Simon McVittie from comment #2)
> This seems reasonable, although I wonder whether it should just not be an
> option, and never fail on ENOENT.

One reason to prefer this is that unknown elements (and, I think, also unknown attributes) prevent the running dbus-daemon from reloading configuration successfully until the next reboot (assuming the system bus).

In Debian or existing derivatives, each package that has moved its system.d snippet to /usr/share would need "Depends: dbus (>= 1.10)" or "Breaks: dbus (<< 1.10)", if we assume this stuff lands before 1.10; otherwise, it would be possible to upgrade the package to a version that cannot actually work, because its system.d snippet is missing.

I considered the approach of "read system.conf from sysconfdir if it exists, or datadir otherwise", but if a sysadmin says "keep my existing system.conf" at the dpkg conffile prompt, then they wouldn't get the effect of subsequent updates to system.conf, or even a dpkg conffile prompt about them. That would be Bad™ because sometimes D-Bus has security vulnerabilities that are fixed by changing system.conf.

(In reply to Simon McVittie from comment #2)
> (In reply to xnox from comment #0)
> > * add support for "ignore_missing" attribute in the includedir element,
> > similar to include element.
> 
> This seems reasonable, although I wonder whether it should just not be an
> option, and never fail on ENOENT. I can't think of any situations in which
> you'd want to distinguish between "drop-in directory is empty" and "drop-in
> directory doesn't exist at all" - can you?
> 

I believe it is silly to be an option. In fontconfig it is an option, and every single <include> of a directory methodically repeats "include_missing=yes". And
for example, in a base-files-like package I have seen people do "mkdir -p /etc/dbus-1/system.d # for dbus". That made me cringe.

I'm all for it to be the default. The patch will be easier then.

> > In my case, I simply use ./configure --sysconfdir=/usr/share to achieve the
> > switch and ship all configuration in /usr/share/dbus-1/ rather than in /etc.
> > And this works very well.
> 
> I don't like this. Configuring sysconfdir = /etc is The Right Thing, even if
> we want to allow it to be empty.
> 

Sure.

> If we make dbus-daemon 1.y read ${datadir}/dbus-1/system.conf, then I think
> it should *always* read ${datadir}/dbus-1/system.conf (so we can document it
> sensibly), rather than being a per-distribution choice.
> 
> Here's a possible design sketch (I'm using the system bus as my example
> throughout, but the same applies equally to the session bus).
> 
> * dbus-daemon --system reads ${datadir}/dbus-1/system.conf,
>   which we can guarantee exists in any non-faulty installation
> 
> * ${datadir}/dbus-1/system.conf is the current bus/system.conf.in,
>   with variable substitutions applied, and with more includes:
> 
>   - does the semi-hard-coded setup (everything above the first
>     <includedir/>)
>   - then includes ${sysconfdir}/dbus-1/system.conf if it exists
>   - then system.d/ as a directory if it exists
>   - then ${sysconfdir}/dbus-1/system.d/ if it exists
>   - then ${sysconfdir}/dbus-1/system-local.conf if it exists
>   - then whatever this contexts/dbus_contexts thing is, in one or
>     both paths (do SELinux distros actually ship this? is it
>     sysadmin configuration or distro configuration or what?
>     maybe Red Hat people would know...)
> 

The matroshka sequence above makes sense to me. and reasoning below.

> * We install a new bus/etc-dbus-1/system.conf.in (or something) which will
>   land in /etc by default (so that the conffile mechanisms in Debian and its
>   derivatives offer to replace the current one, handling user modifications
>   correctly); it is functionally empty (I think in practice it needs
>   a <busconfig></busconfig>), and has comments saying that it is
>   deprecated, and you should create system.d/local.conf or system-local.conf
>   if you want to adjust the bus config
> 
> * For regression tests, instead of copying bus/system.conf into the test
>   data directory unmodified, we redo the substitutions, replacing
>   ${sysconfdir} with the installed-test data directory
>   ${testexecdir}/data, to avoid using the actual system copies here
> 

So do you want above a direct patch to switch (and then choose appropriate branch/time to apply), or a config option "--with-new-world-order-configuration-locations" ?!

> We can eventually delete /etc/dbus-1/system.conf entirely, and embedded
> distros can do so if they want to, but I think it has existed for so long (~
> 10 years) that replacing it with a comment saying "this is deprecated" is
> the more friendly approach.
> 
> AIUI, the difference between system.conf and system-local.conf is that RPM
> distributions forcibly replace the former during upgrades (there is never a
> dpkg-style conffile prompt in RPM-land), and never touch the latter: which,
> for those with a more Debian mindset, means system.conf should really have
> been in ${datadir}, and system-local.conf in ${sysconfdir}.
> 
> Rationale for that specific search path:
> 
> * Distro snippets in system.d have historically taken precedence over
> system.conf. They should continue to do so when moved to the datadir.
> 
> * Everything else being equal, /etc should generally take precedence over
> /usr.
> 
> * system-local.conf makes no sense in ${datadir}, because it is explicitly
> for the local sysadmin, not for the OS integrator.
> 
> * system-local.conf has the highest precedence, by design...
> 
> * ... but whatever this SELinux context thing is has traditionally had even
> higher precedence than system-local.conf.
> 
> If a sysadmin on a Debian derivative has modified /etc/dbus-1/system.conf,
> and keeps their modified version, then their dbus-daemon will redundantly
> parse /etc/dbus-1/system.d and /etc/dbus-1/system-local.conf twice - but I
> don't think that's the end of the world.

(In reply to xnox from comment #4)
> So do you want above a direct patch to switch (and then choose appropriate
> branch/time to apply), or a config option
> "--with-new-world-order-configuration-locations" ?!

I think we want a direct patch to just do the switch, for master (currently 1.9.x), installing the real system.conf to /usr/share/dbus-1/system.conf, and by default installing a new stub config file (empty <busconfig> + comment about how it's deprecated) to /etc/dbus-1/system.conf.

Hmm... this is potentially going to break Ubuntu, which installs dbus-daemon to /bin. Unless Ubuntu doesn't actually support a separate /usr not being mounted by the initramfs, in which case we're fine?

(In reply to Simon McVittie from comment #5)
> (In reply to xnox from comment #4)
> > So do you want above a direct patch to switch (and then choose appropriate
> > branch/time to apply), or a config option
> > "--with-new-world-order-configuration-locations" ?!
> 
> I think we want a direct patch to just do the switch, for master (currently
> 1.9.x), installing the real system.conf to /usr/share/dbus-1/system.conf,
> and by default installing a new stub config file (empty <busconfig> +
> comment about how it's deprecated) to /etc/dbus-1/system.conf.
> 
> Hmm... this is potentially going to break Ubuntu, which installs dbus-daemon
> to /bin. Unless Ubuntu doesn't actually support a separate /usr not being
> mounted by the initramfs, in which case we're fine?

On ubuntu, it would break under upstart where /usr happens to be on a non-local (e.g. networked) filesystem. At the moment dbus is started after local but before remote. I'll test if this actually works in-practice with upstart and systemd in vivid - since interfaces & system-services are in /usr/share... The claim is that ubuntu needs dbus in early boot, however I don't believe this is true any more. (we nee libdbus in early boot, but not dbus-daemon itself). The first reference of the move is from Keybuk in 2008 - it states that it was moved, but not why ("start earlier..."). Digging into current structure I see that everything that start on started dbus is in /usr.... including connman & network-manager. I'm confident that systemd boot on Ubuntu works with dbus relying on /usr, but I will verify that upstart boot-path works as well. Possibly changing local-filesystem -> remote-filesystem or the /usr mount point event.

(In reply to xnox from comment #6)
> On ubuntu, it would break under upstart where /usr happens to be on a
> non-local (e.g. networked) filesystem. At the moment dbus is started after
> local but before remote. I'll test if this actually works in-practice with
> upstart and systemd in vivid - since interfaces & system-services are in
> /usr/share...

Yeah, I've always been rather suspicious about the claim that dbus-daemon can usefully start before /usr. With systemd, we can have the socket listening before the actual dbus-daemon becomes available, which seems like the best of both worlds.

The interface XML in /usr/share is not needed during boot - the only time it might conceivably be needed is while compiling things - but the contents of system-services are certainly needed before the system dbus-daemon is much use to anyone.

I suspect that Ubuntu just doesn't work properly with NFS /usr. It is not clear to me unclear why you'd want to do that in any case, given the possibility of either local root or NFS root, but it seems "this doesn't make sense" has never been sufficient reason for people to avoid particular boot configurations.

> The claim is that ubuntu needs dbus in early boot, however I
> don't believe this is true any more. (we nee libdbus in early boot, but not
> dbus-daemon itself).

Right, Debian installs libdbus to /lib but the rest is in /usr. If you have influence over Ubuntu's dbus, I'd like to see it sync up with Debian's by building with exec_prefix=/usr (possibly moving dbus-daemon, dbus-cleanup-sockets and/or dbus-uuidgen to /bin if that's necessary for backwards compatibility, but it's better for those to be the exception rather than the rule IMO, because dbus-test-tool, dbus-update-activation-environment and any other binaries I add in future are much more likely to be /usr material).

Created attachment 113819 [details] [review]
[1/2] Make include_dir non-existing directory, to not be an error.

Created attachment 113820 [details] [review]
[2/2] Make include_dir non-existing directory, to not be an error.

Created attachment 113821 [details] [review]
[2/2]  Move session & system bus configuration to datadir, by default.

I hope I did this right.

1/2 -> is now trivial, and can be merged straight away.

2/2 -> I hope I did it right:

- only for autoconf build system

- i didn't do "substitutions" for installed tests, instead i change the symlinks to point at %{datadir}, and the file continue to use relative paths, thus it should still work.

Comment on attachment 113821 [details] [review]
[2/2]  Move session & system bus configuration to datadir, by default.

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

I would like the CMake-land configuration to be at least broadly consistent with Autotools: it should install the main session.conf, system.conf to the same places that Autotools does.

It's OK if it doesn't install the backwards-compat version, and it's OK if it's less flexible than the Autotools build (e.g. doesn't support a ${datadir} that isn't ${prefix}/share).

::: bus/Makefile.am
@@ +1,1 @@
> +configdir=$(datadir)/dbus-1

I'd prefer dbusdatadir or something: it seems needlessly confusing to have a variable named "configdir" that does not point to the sysconfdir.

It's OK that we outsource our "built-in" setup (conceptually part of dbus) to files that are considered to be "OS data" rather than "sysadmin configuration", but it seems a stretch to call it "configuration" if we're treating it as officially non-configurable.

::: bus/legacy-config/session.conf
@@ +1,5 @@
> +<!-- THIS FILE MAY BE SAFELY REMOVED -->
> +
> +<!-- This _legacy_ configuration file _used to_ control the sessionwide message bus.
> +
> +     The real one is, usually at /usr/share/dbus-1/session.conf, but don't edit it.

Adding a comma here is, not correct grammar :-)

I'd prefer something more like this (and the same with s/session/system/
in the other one), with the usual .in substitutions:

<!--
This configuration file is no longer required and may be removed.

In older versions of dbus, this file defined the behaviour of the well-known
session bus. That behaviour is now determined by
@DBUS_DATADIR@/dbus-1/session.conf, which should not be edited.

For local configuration changes, create a file
@DBUS_SYSCONFDIR@/dbus-1/session-local.conf or files matching
@DBUS_SYSCONFDIR@/dbus-1/session.d/*.conf, with a <busconfig>
element containing configuration directives. These directives can
override D-Bus or OS defaults.

For upstream or distribution-wide defaults that can be overridden
by a local sysadmin, create files matching
@DBUS_DATADIR@/dbus-1/session.d/*.conf instead.
-->

and for the system bus the last paragraph could be something like

For upstream or distribution-wide defaults that can be overridden
by a local sysadmin, including security policy entries to allow system
services to work, create files matching
@DBUS_DATADIR@/dbus-1/system.d/*.conf instead.

@@ +12,5 @@
> +     If you are upstream/distribution, please ship session.d/* snippets instead in:
> +
> +     $ pkg-config --variable=session_bus_config_dir dbus-1
> +
> +     ( usually /usr/share/dbus-1/session.d )

I don't think we should advise upstreams to install to a pkg-config'd path. They should install to their own ${datadir}/dbus-1/session.d, and OS integrators (or sysadmins) who want to support alternative paths are responsible for arranging for those paths to be searched. See also <http://lists.freedesktop.org/archives/systemd-devel/2014-October/024388.html>.

Perhaps this means the default setup for the system bus should search /usr/local/share/dbus-1/system.d and /usr/share/dbus-1/system.d before it reads its own ${datadir}/dbus-1/system.d, with some sort of deduplication for the common case of --prefix=/usr so that directories that were already searched during this reload attempt are not searched again.

Similarly, perhaps the session bus should search XDG_CONFIG_DIRS/dbus-1/session.d and/or XDG_DATA_DIRS/dbus-1/session.d (as defined by the XDG basedir spec), before looking in its own ${prefix} (if different)? We'd need a new directive <standard_session_configdirs/> similar to the existing <standard_session_servicedirs/> to set where that appears in the search order, but it would close a long-standing feature request in Debian.

I'm less concerned about breaking the ability to reload the session bus, because in practice, the session bus doesn't actually have drop-in snippets like the system bus on general-purpose Linux - its default-allow policy makes that unnecessary.

::: dbus/Makefile.am
@@ +1,2 @@
>  
> +configdir=$(datadir)/dbus-1

Same comments as before

::: test/Makefile.am
@@ +434,5 @@
>  		install -d "$(DESTDIR)$(testexecdir)/$${F%/*}"; \
>  		install -m644 "installable/$${F%.in}" "$(DESTDIR)$(testexecdir)/$${F%.in}"; \
>  	done
> +	ln -nfs $(datadir)/dbus-1/session.conf $(DESTDIR)$(testexecdir)/data/valid-config-files/session.conf
> +	ln -nfs $(datadir)/dbus-1/session.d $(DESTDIR)$(testexecdir)/data/valid-config-files/session.d

Now that <includedir>this-doesnt-exist.d</includedir> is not an error, you don't really need this...

@@ +436,5 @@
>  	done
> +	ln -nfs $(datadir)/dbus-1/session.conf $(DESTDIR)$(testexecdir)/data/valid-config-files/session.conf
> +	ln -nfs $(datadir)/dbus-1/session.d $(DESTDIR)$(testexecdir)/data/valid-config-files/session.d
> +	ln -nfs $(datadir)/dbus-1/system.conf $(DESTDIR)$(testexecdir)/data/valid-config-files/system.conf
> +	ln -nfs $(datadir)/dbus-1/system.d $(DESTDIR)$(testexecdir)/data/valid-config-files/system.d

... or this.

::: tools/Makefile.am
@@ +1,1 @@
> +configdir=$(datadir)/dbus-1

Doesn't actually seem to be used?

Comment on attachment 113819 [details] [review]
[1/2] Make include_dir non-existing directory, to not be an error.

Applied, thanks

Thiago, Colin, any opinions on this?

(tl;dr: moving the canonical home for {session,system}.conf and upstream/OS vendor {session,system}.d snippets to ${datadir}/dbus-1, with backwards compat for the versions in /etc/dbus-1)

Comment on attachment 113821 [details] [review]
[2/2]  Move session & system bus configuration to datadir, by default.

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

using the clickly ui for the first time. i hope this publishes the replies to comments.

::: bus/Makefile.am
@@ +1,1 @@
> +configdir=$(datadir)/dbus-1

sure, i went for least lines of change. I can change it to dbusdatadir for what's its worth.

::: bus/legacy-config/session.conf
@@ +1,5 @@
> +<!-- THIS FILE MAY BE SAFELY REMOVED -->
> +
> +<!-- This _legacy_ configuration file _used to_ control the sessionwide message bus.
> +
> +     The real one is, usually at /usr/share/dbus-1/session.conf, but don't edit it.

english, mate, i don't speak it. </pulp fiction>

@@ +12,5 @@
> +     If you are upstream/distribution, please ship session.d/* snippets instead in:
> +
> +     $ pkg-config --variable=session_bus_config_dir dbus-1
> +
> +     ( usually /usr/share/dbus-1/session.d )

i added that to match others.
systemd upstream at the moment queries sysconfdir from dbus-1.pc and installs into there/dbus-1/system.d/
that's the reason for adding a new variable name, such that they could query that variable and if present install into there (that is under datadir)
most other upstreams simply install into whatever they are configured to treat as sysconfdir, or simply install stuff into /etc direct.
What i was after here is to provide a "flag" that upstreams can use as an indicator that they should start installing things into /usr/share/, otherwise nobody will move to /usr =)
Anyway i'm happy to drop this and respective change to .pc.in and for somebody more senssible in dbus upstream to do that change, if any are needed.

(In reply to Simon McVittie from comment #14)
> Thiago, Colin, any opinions on this?
> 
> (tl;dr: moving the canonical home for {session,system}.conf and upstream/OS
> vendor {session,system}.d snippets to ${datadir}/dbus-1, with backwards
> compat for the versions in /etc/dbus-1)

Makes sense to be, my high level concern here is that there's a lot of fragility in this process, particularly the interaction with upgrades, so we'll just have to watch out for that.

I definitely know of installations that bump some of the random hardcoded limits in system.conf and session.conf for example.  As long as we interact well with proper upgrade systems that preserve modified config files, I'm OK with it.

(In reply to xnox from comment #15)
> i added that to match others.
> systemd upstream at the moment queries sysconfdir from dbus-1.pc and
> installs into there/dbus-1/system.d/

Not any more:

http://cgit.freedesktop.org/systemd/systemd/tree/configure.ac#n1376

(but I'm sure the older versions in various distributions still look for dbus' sysconfdir).

> most other upstreams simply install into whatever they are configured to
> treat as sysconfdir, or simply install stuff into /etc direct.

I consider this to be correct behaviour. A package configured for a particular ${prefix} should install there, and it's up to the system integrator (or sysadmin) to make sure their packages interoperate.

> What i was after here is to provide a "flag" that upstreams can use as an
> indicator that they should start installing things into /usr/share/,
> otherwise nobody will move to /usr =)

Assuming we merge this in the 1.9 series, upstreams cannot make this change unconditionally until dbus 1.10 is widespread. This is not going to happen overnight.

If upstreams are going to make this change conditionally, I would prefer a --with option like the one in systemd git master (which I think is the ideal thing tbh, since it lets packagers force the issue), or having the switch be either "dbus is >= 1.10" (which means the old paths stick around until then, but that's fine IMO) or "${datadir}/dbus-1/system.conf exists at configure time".

If an upstream defaults to installing policy snippets in ${sysconfdir}, Debian derivatives can easily move the policy snippets around with a properly-written debian/foo.install. Other distributions might have to resort to doing some "mv" commands after "make install", but that's not the end of the world.

(In reply to Colin Walters from comment #16)
> I definitely know of installations that bump some of the random hardcoded
> limits in system.conf and session.conf for example.

Those installations should ideally have obeyed the instruction to "Add a system-local.conf and edit that rather than changing this file directly".

Fedora uses "%config %{_sysconfdir}/dbus-1/*.conf" rather than %config(noreplace), which I think means sysadmin changes to {system,session}.conf will not be preserved in any case.

I wouldn't mind installing an example system-local.conf to ${docdir}/examples, and pointing to it in a comment, if you think that would help.

> As long as we interact
> well with proper upgrade systems that preserve modified config files, I'm OK
> with it.

All of this only applies when the sysadmin has ignored the instruction to not modify that file: both dpkg and rpm would silently replace an unmodified old file with the new one.

1. noreplace: The worst case is that s*.conf is %config(noreplace) (as in at least Tizen, possibly inherited from SUSE) and we unconditionally keep the old version, resulting in the default security policy being applied twice, /etc/dbus-1/s*.d being included twice, and /etc/dbus-1/s*-local.conf being included twice. I think that's actually OK: the only trap here is that the default security policy in the old /etc/dbus-1/s*.conf will be applied with higher precedence than the default security policy in /usr/share/dbus-1/s*.conf.

However, if your distribution is unconditionally keeping old config without notifying the sysadmin that something has changed and they should take a look at it, then from the perspective of being able to fix security vulnerabilities by adjusting the config file, you've already lost anyway.

2. overwrite: The ideal case from the point of view of our upgrades (although not from sysadmins' point of view) is %config (as seen in Fedora), in which we unconditionally trample over the old file - but recent D-Bus security updates have also had to do that, so sysadmins should be used to this.

3. prompt to overwrite or not: In dpkg-based distributions, by default there's a conffile prompt if both the sysadmin and the packager have altered the file. The sysadmin can either go the %config(noreplace)-like route (default), or the %config-like route, or inspect the changes and merge them. A secondary purpose of this prompt is to flag up the change as something they should look at - and when they do that, they'll see the comments in the new file as part of the diff, and can behave accordingly.

One possibility to improve on (1) would be to move out the "meat" of the defaults file (the default security policy) into a separate /usr/share/dbus-1/system-security-policy.conf (or something), have /usr/share/dbus-1/system.conf just be a series of <include>s, and <include> system-security-policy.conf after /etc/dbus-1/system.conf but before /etc/dbus-1/system.d. But I think that's unnecessary complexity tbh.

Created attachment 113917 [details] [review]
[1/2] Move session & system bus configuration to datadir, by default.

Created attachment 113918 [details] [review]
[2/2] Adjust cmake build to match autoconf installation locations.

Dropped all changes to pkg-config files, so they stay the same.

Use suggested wording for the legacy config files.

Adjusted cmake build-system (in a separate patch) to match the autoconf installation, possibly breaking or fixing odd things about the cmake build. Someone should test it on windows, as i have no windows dev environment to test it out.

Ubuntu now ships dbus-daemon in /usr/bin.

Comment on attachment 113917 [details] [review]
[1/2] Move session & system bus configuration to datadir, by default.

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

This looks basically good, although I'd like to do more testing before I merge it.

::: bus/Makefile.am
@@ +1,2 @@
> +dbusdatadir=$(datadir)/dbus-1
> +legacydbusdatadir=$(sysconfdir)/dbus-1

I'm not sure why this doesn't remain called configdir, but it isn't a big deal.

Comment on attachment 113918 [details] [review]
[2/2] Adjust cmake build to match autoconf installation locations.

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

::: cmake/CMakeLists.txt
@@ +75,5 @@
>  set(DBUS_MACHINE_UUID_FILE   ${DBUS_INSTALL_DIR}/lib/dbus/machine-id)
>  set(DBUS_BINDIR              ${EXPANDED_BINDIR})
>  set(DBUS_DAEMONDIR           ${EXPANDED_BINDIR})
> +set(DBUS_LOCALSTATEDIR       /var)
> +set(DBUS_SYSCONFDIR          /etc)

I don't think hard-coding these is right. cmake installations should default to being under a ${prefix} just like Autotools.

If these can be overridden from the command line (e.g. via -DDBUS_LOCALSTATEDIR=/var -DDBUS_SYSCONFDIR=/etc) then that's as good as Autotools' --sysconfdir=/etc.

::: cmake/bus/CMakeLists.txt
@@ -8,4 @@
>  FILE(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/session.d)
>  
>  if(NOT WIN32)
> -    CONFIGURE_FILE( "system.conf.cmake" "${CMAKE_CURRENT_BINARY_DIR}/system.conf" IMMEDIATE @ONLY)

Replacing system.conf.cmake with a non-cmake-specific version is certainly an improvement.

Created attachment 116061 [details]
[3/3] Use /var & /etc, relative DBUS_INSTALL_DIR in cmake.

This is the only fixup of the review comment of the 2/2 patch.
Feel free to squeesh the two together.
Didn't feel like regenerating the other two patches, as they are unchanged, and rebase onto recent codebase cleanly.

Can this be merged please? I have been using this in https://clearlinux.org without any issues for a long time now.

(In reply to xnox from comment #24)
> Can this be merged please? I have been using this in https://clearlinux.org
> without any issues for a long time now.

For one horrible moment I didn't remember the context and thought you meant you were using the CMake build system for ClearLinux... but thankfully not.

I still need to verify how this interacts with packaging systems (dpkg) by generating and packaging a snapshot, but in principle the answer is "yes, I'll merge it".

(In reply to xnox from comment #21)
> Ubuntu now ships dbus-daemon in /usr/bin.

I saw, and I approve.

(In reply to Simon McVittie from comment #25)
> (In reply to xnox from comment #24)
> > Can this be merged please? I have been using this in https://clearlinux.org
> > without any issues for a long time now.
> 
> For one horrible moment I didn't remember the context and thought you meant
> you were using the CMake build system for ClearLinux... but thankfully not.
> 

Haha =)


> I still need to verify how this interacts with packaging systems (dpkg) by
> generating and packaging a snapshot, but in principle the answer is "yes,
> I'll merge it".
> 
> (In reply to xnox from comment #21)
> > Ubuntu now ships dbus-daemon in /usr/bin.
> 
> I saw, and I approve.

Created attachment 116073 [details] [review]
dbus-daemon.1: document the new locations

Created attachment 116074 [details] [review]
tests: use the new bus setup for `make installcheck`

---

Otherwise it fails, because $(DESTDIR)/etc/dbus-1/s*.conf are no longer suitable values for the --config-file option.

Comment on attachment 116073 [details] [review]
dbus-daemon.1: document the new locations

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

looks good to me.

Upgrade-test result:

One slightly unexpected result is that reloading the bus configuration with SIGHUP fails until the next reboot, because the new default /etc/dbus-1/session.conf no longer contains a <listen> directive, hence it is no longer considered valid. However, we already recommend a reboot after dbus upgrades, because the new dbus-daemon executable will not be used until after the next reboot anyway; so I don't think that's necessarily a problem, particularly for a major-version upgrade (dbus 1.8 to dbus 1.10).

Packagers obviously need to alter packaging, but that's normal for a development branch, and the changes aren't extensive.

codesearch.debian.net result:

<https://codesearch.debian.net/results/dbus-1%2Fsession.conf/page_0>
<https://codesearch.debian.net/results/dbus-1%2Fsystem.conf/page_0>

systemd-bus-proxyd will need reconfiguration. That's for kdbus, which isn't stable anyway, so no big deal.

tcos, a framework for thin clients, is second-guessing what's in the dbus package, which will never work reliably across upstream major version changes in any case - what if we added a library dependency? - so I don't feel guilty about breaking it in a development branch.

The rest are false positives (test data).

So I think we're good, if someone reviews Attachment #116074 [details].

Comment on attachment 116074 [details] [review]
tests: use the new bus setup for `make installcheck`

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

I have verified this patch, it is necessory for this patch series. With it check/installcheck pass without regressions, as compared to the master branch without this patch series.

First reproduced base line of all check/installcheck tests passing on Ubuntu 14.04 container.

Then applied full patch series, and verified that all check/installcheck tests are passing.

Then reverted this last patch, and verified that tests hung / abort / fail.

Reviewed outstanding patches from Simon, part of this patch series.

Fixed in git for 1.9.18, thanks!