Bug 45520

Created attachment 56483 [details] [review]
systemd: drop machine UUID generation for unit file

Created attachment 56484 [details] [review]
systemd: don't remove PID file explicitly in unit file

(In reply to comment #1)
> systemd: drop machine UUID generation for unit file

Looks fine, fixed in git for 1.5.10.

(In reply to comment #2)
> systemd: don't remove PID file explicitly in unit file

If the system dbus-daemon crashes, would systemd ever attempt to restart it? If it would, with this change it'd fail, because the new dbus-daemon won't cope with the orphaned pid file (Bug #45713).

I believe the official line is still that restarting the system dbus-daemon is unsupported, though, so perhaps that doesn't matter.

Or, since the pid file is basically useless under systemd, one way to deal with that would be to add a --nopidfile option (which would override system.conf, just like --nofork does) and use it in dbus.service.in.

With hindsight, <fork/> and <pidfile/> should never have been in system.conf - we should have just said that sysvinit-style init scripts should specify "--system --fork --pidfile=..." - but that'd be an incompatible change for traditional-init systems now, so we're basically stuck with it.

Happily, the systemd unit is in dbus itself, so we can do The Right Thing with command-line options there.

(In reply to comment #3)

> (In reply to comment #2)
> > systemd: don't remove PID file explicitly in unit file
> 
> If the system dbus-daemon crashes, would systemd ever attempt to restart it? If
> it would, with this change it'd fail, because the new dbus-daemon won't cope
> with the orphaned pid file (Bug #45713).
> 
> I believe the official line is still that restarting the system dbus-daemon is
> unsupported, though, so perhaps that doesn't matter.
> 
> Or, since the pid file is basically useless under systemd, one way to deal with
> that would be to add a --nopidfile option (which would override system.conf,
> just like --nofork does) and use it in dbus.service.in.
> 
> With hindsight, <fork/> and <pidfile/> should never have been in system.conf -
> we should have just said that sysvinit-style init scripts should specify
> "--system --fork --pidfile=..." - but that'd be an incompatible change for
> traditional-init systems now, so we're basically stuck with it.
> 
> Happily, the systemd unit is in dbus itself, so we can do The Right Thing with
> command-line options there.

On Fedora at least we explicitly don't support restarting D-Bus. But you are right --nopidfile might be a nice fix here. I'll look into that.

Created attachment 56844 [details] [review]
bus: introduce --nopidfile switch to disable writing of PID files

OK, here's a patch that introduces --nopidfile and makes use of it in the systemd unit file.

Renaming bug to reflect more appropriately what this is about now.

Comment on attachment 56844 [details] [review]
bus: introduce --nopidfile switch to disable writing of PID files

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

Applied, thanks.

::: bus/bus.c
@@ +270,4 @@
>  				BusConfigParser  *parser,
>                                  const DBusString *address,
>                                  dbus_bool_t      systemd_activation,
> +                                dbus_bool_t      write_pidfile,

Ah, the "multiple boolean arguments" anti-pattern.

I'm very tempted to turn this into a flags argument for clarity (BUS_CONTEXT_WRITE_PIDFILE | BUS_CONTEXT_SYSTEMD_ACTIVATION) but for now I'll just apply your patch.

Created attachment 56862 [details] [review]
Replace a series of booleans, all (apparently) alike, with  flags

This makes it a bit clearer what's going on.

---

How's this?

(In reply to comment #8)
> Created attachment 56862 [details] [review] [review]
> Replace a series of booleans, all (apparently) alike, with  flags
> 
> This makes it a bit clearer what's going on.
> 
> ---
> 
> How's this?

Looks good to me! Thanks for reworking this!

Fixed in git for 1.5.10 (including my extra patch - thanks for reviewing)