Previously, dbus-launch passed --fork to dbus-daemon, which (among other things) causes it to open /dev/null for stdin/stdout/stderr. Then, session components activated by dbus-daemon simply inherit these defaults. In a systemd environment, we really want the output of everything to be hooked up to the journal by default. At present, gdm launches dbus-daemon for the user session, and I will be modifying it to pass this flag by default. This solves the "Where the $#%@ are my error messages?" issue I was having while debugging something else... --- tools/dbus-launch.c | 65 +++++++++++++++++++++++++++++++------------------- 1 files changed, 40 insertions(+), 25 deletions(-)
Created attachment 79633 [details] [review] 0001-dbus-launch-Add-an-option-no-bus-fork.patch
See also https://bugzilla.gnome.org/show_bug.cgi?id=700816
At a high level, I know that with this patch, I'm basically taking code that was already spaghetti and cutting each slice in two, then dumping marinara sauce on top just to obscure it further. You'd be well within your rights to say "screw this, you should be using systemd-for-the-session" which (as far as I understand) would launch dbus-daemon as a systemd unit connected to the journal. Which would indeed be nicer, I have to admit...
(In reply to comment #3) > At a high level, I know that with this patch, I'm basically taking code that > was already spaghetti and cutting each slice in two, then dumping marinara > sauce on top just to obscure it further. I'm really starting to hate dbus-launch. If you're going to change it, could you please review the patches on Bug #39196 and Bug #39197 first? I would really like to have dbus-run-session, so we can stop recommending dbus-launch for regression test suites and similar stuff (for which it is not actually very suitable), and can concentrate on keeping dbus-launch backwards-compatible and semi-sane for X11. (Long-term plan: when we're all living in the Wayland-or-possibly-Mir future, I would like dbus-launch to vanish in favour of systemd --user and/or dbus-run-session.) Is there any reason why dbus-daemon needs to re-fork when run from dbus-launch? Can't we run it with --no-fork always, and have dbus-launch take responsibility for closing the child process's standard fds if appropriate before exec'ing dbus-daemon? ("If appropriate" would have to involve a new --no-close-stdfds or something, I think.)
(In reply to comment #0) > Previously, dbus-launch passed --fork to dbus-daemon, which (among > other things) causes it to open /dev/null for stdin/stdout/stderr. I think it should still have /dev/null for stdin. Is there anything else that --fork does, for instance playing with POSIX session IDs or controlling terminals?
(In reply to comment #4) > (In reply to comment #3) > > At a high level, I know that with this patch, I'm basically taking code that > > was already spaghetti and cutting each slice in two, then dumping marinara > > sauce on top just to obscure it further. > > I'm really starting to hate dbus-launch. > > If you're going to change it, could you please review the patches on Bug > #39196 and Bug #39197 first? Done. > Is there any reason why dbus-daemon needs to re-fork when run from > dbus-launch? Can't we run it with --no-fork always, and have dbus-launch > take responsibility for closing the child process's standard fds if > appropriate before exec'ing dbus-daemon? ("If appropriate" would have to > involve a new --no-close-stdfds or something, I think.) True, we could always avoid forking. We could also consider just doing the behavioral change of just making it the default (or at worst, a compile time flag or an option in session.conf). The more I think about it, that's really tempting.
(In reply to comment #5) > (In reply to comment #0) > > Previously, dbus-launch passed --fork to dbus-daemon, which (among > > other things) causes it to open /dev/null for stdin/stdout/stderr. > > I think it should still have /dev/null for stdin. Ack. > Is there anything else that --fork does, for instance playing with POSIX > session IDs or controlling terminals? It's all in _dbus_become_daemon() - yes, we call setsid(), chdir to ("/"), set umask(). I don't think being in a different session group really matters for the login session. However, could some session activated service actually depend on the cwd being /? Maybe...I'd argue it'd be broken, but who knows. I guess if we were really paranoid we could change the activation helper to chdir("/"). Though the umask one is an interesting topic... http://pthree.org/2007/10/24/default-umask-in-debian/ Oh but session.conf already has: <keep_umask/> So this wouldn't be a behavioral change there. Though it's interesting to note that the system bus on Debian causes activated processes to have a different umask from the default.
(In reply to comment #6) > True, we could always avoid forking. We could also consider just doing the > behavioral change of just making it the default (or at worst, a compile time > flag or an option in session.conf). The default, if you just run dbus-daemon, is not to fork. You can in fact put <fork/> in session.conf, but it's a terrible idea. dbus-launch, and tools like it, should always explicitly pass either --fork or --nofork, otherwise a misguided sysadmin changing session.conf could break their expectations. (In reply to comment #7) > I don't think being in a different session group really > matters for the login session. We'd have to read up on what setsid() actually does... > However, could some session activated > service actually depend on the cwd being /? Maybe...I'd argue it'd be > broken, but who knows. I guess if we were really paranoid we could change > the activation helper to chdir("/"). The activation helper isn't run for the session bus, only for the system bus. I wouldn't mind doing a chdir("/"), maybe a setsid(), and reopening /dev/null to stdin in the dbus-launch child just before it exec()s the dbus-daemon. dbus-launch.c is Unix-only anyway. > Though it's interesting to > note that the system bus on Debian causes activated processes to have a > different umask from the default. 022 seems a sane default for system services, tbh. I'd put this under the heading of "good system services should not make assumptions about their inherited environment".
> * If you are in a shell and run "dbus-launch myapp", here is what happens: > * > * shell [*] > * \- main() --exec--> myapp[*] > * \- "intermediate parent" > * \- bus-runner --exec--> dbus-daemon --fork > * \- babysitter[*] \- final dbus-daemon[*] > * > * Processes marked [*] survive the initial flurry of activity. > * > * If you run "dbus-launch --sh-syntax" then the diagram is the same, except > * that main() prints variables and exits 0 instead of exec'ing myapp. Whatever you end up with, we should keep this diagram up to date. > - switch (read_pid (read_bus_pid_fd, &bus_pid_to_kill)) > + if (no_bus_fork) If having the dbus-daemon fork is optional, you could reduce the spaghetti considerably by always reading the pid from the pipe, even when we already know it because it's our own child process... but if we switch to always using --nofork, that's unnecessary.
Created attachment 80365 [details] [review] dbus-launch: run dbus-daemon with --nofork Instead of running dbus-daemon --fork, do the 75% of it that we actually want. Under systemd, we want to leave stderr open, so that any stderr from either dbus-daemon or the services it activated will be logged to the journal; under non-systemd, stderr will either be somewhere useful, or already /dev/null. --- Untested, might not work. At some point I'll test this on a live Debian system.
I don't really need this anymore. Full steam ahead on https://wiki.gnome.org/ThreePointEleven/Features/SystemdUserSession
WONTFIX then. Hopefully one day dbus-launch will be a thing of the past.
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.