Bug 99162

Comment on attachment 128595 [details] [review]
the patch

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

::: bus/dbus.conf
@@ +1,5 @@
> +# sysusers.d snippet for creating the D-Bus system user automatically
> +# at boot on systemd-based systems that ship with an unpopulated
> +# /etc. See sysusers.d(5) for details.
> +
> +u dbus - "System Message Bus"

Instead of hardcoding `dbus`, this needs to use the value from configure’s --with-dbus-user.

ah, indeed. I wasn't aware this was configurable.

Created attachment 128641 [details] [review]
new patch, this time honours the user name configured at configure time

Comment on attachment 128641 [details] [review]
new patch, this time honours the user name configured at configure time

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

Looks like it distributes dbus.conf and dbus.conf.in?

::: bus/Makefile.am
@@ +318,4 @@
>  	dbus.socket.in \
>  	systemd-user/dbus.service.in \
>  	systemd-user/dbus.socket.in \
> +	dbus.conf \

I think this should probably be `dbus.conf.in`?

(In reply to Philip Withnall from comment #4)
> Comment on attachment 128641 [details] [review] [review]
> new patch, this time honours the user name configured at configure time
> 
> Review of attachment 128641 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks like it distributes dbus.conf and dbus.conf.in?
> 
> ::: bus/Makefile.am
> @@ +318,4 @@
> >  	dbus.socket.in \
> >  	systemd-user/dbus.service.in \
> >  	systemd-user/dbus.socket.in \
> > +	dbus.conf \
> 
> I think this should probably be `dbus.conf.in`?

True. And worse: the dbus.conf.in shouldn't have been listed in CONFIG_IN_FILES=!

Created attachment 128846 [details] [review]
third version of this patch

fix the .in confusion

Comment on attachment 128846 [details] [review]
third version of this patch

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

Looks good to me, thanks!

Not sure I can push it, so this might have to wait for smcv to ack it, but from my pov this is review+.

Comment on attachment 128846 [details] [review]
third version of this patch

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

::: bus/Makefile.am
@@ +1,4 @@
>  dbusdatadir=$(datadir)/dbus-1
>  legacydbusdatadir=$(sysconfdir)/dbus-1
>  dbus_daemon_execdir = $(DBUS_DAEMONDIR)
> +systemdsysusersdir = $(libdir)/sysusers.d

On multilib systems (those with lib64) and Debian-style multiarch systems (those with lib/x86_64-linux-gnu or whatever) this is not going to be where systemd looks for the files, I don't think?

Shouldn't it be more like this?

# Always "lib", even on systems with lib64 or multiarch
systemdsysusersdir = $(prefix)/lib/sysusers.d

@@ +324,5 @@
>  	dbus.service \
>  	dbus.socket
> +
> +nodist_systemdsysusers_DATA = \
> +	dbus.conf

I'd slightly prefer to use bus/sysusers.d/dbus.conf{,.in}. Otherwise this file would collide with any other configuration drop-in that is owned by dbus and happens to also use the ".conf" extension.

(In reply to Simon McVittie from comment #8)

> @@ +1,4 @@
> >  dbusdatadir=$(datadir)/dbus-1
> >  legacydbusdatadir=$(sysconfdir)/dbus-1
> >  dbus_daemon_execdir = $(DBUS_DAEMONDIR)
> > +systemdsysusersdir = $(libdir)/sysusers.d
> 
> On multilib systems (those with lib64) and Debian-style multiarch systems
> (those with lib/x86_64-linux-gnu or whatever) this is not going to be where
> systemd looks for the files, I don't think?

Indeed, of course!

> I'd slightly prefer to use bus/sysusers.d/dbus.conf{,.in}. Otherwise this
> file would collide with any other configuration drop-in that is owned by
> dbus and happens to also use the ".conf" extension.

OK, will fix.

Created attachment 129439 [details] [review]
fourth version of the patch

This time with the path, and moved down one dir

Comment on attachment 129439 [details] [review]
fourth version of the patch

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

::: bus/Makefile.am
@@ +1,4 @@
>  dbusdatadir=$(datadir)/dbus-1
>  legacydbusdatadir=$(sysconfdir)/dbus-1
>  dbus_daemon_execdir = $(DBUS_DAEMONDIR)
> +systemdsysusersdir = $(prefix)/lib/sysusers.d

I'd like a comment above this so that future maintainers don't "correct" this to what you initially said:

# Always .../lib, even on systems where ${libdir} is .../lib64 or .../lib/x86_64-linux-gnu

or something like that.

Looks good to merge. I'd prefer it with that comment but it isn't required.

(If you don't get there first, I'll add the suggested comment and apply this at some point.)

Fixed in git for 1.11.10, thanks

Excellent! Thanks for merging!