Bug 39196

Not only is exiting with its child not what dbus-launch is designed to do, dbus-launch's process/code structure more or less prevents it. Here is a diagram which I'd like to add to dbus-launch.c so future maintainers can understand it more easily:

/* PROCESSES
 *
 * 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.
 *
 * PIPES
 *
 * dbus-daemon --print-pid     -> bus_pid_to_launcher_pipe     -> main
 * dbus-daemon --print-address -> bus_address_to_launcher_pipe -> main
 * main                        -> bus_pid_to_babysitter_pipe   -> babysitter
 *
 * The intermediate parent looks pretty useless at first glance. Its purpose
 * is to avoid the bus-runner becoming a zombie: when the intermediate parent
 * terminates, the bus-runner and babysitter are reparented to init, which
 * reaps them if they have finished. We can't rely on main() to reap arbitrary
 * children because it might exec myapp, after which it can't be relied on to
 * reap its children. We *can* rely on main() to reap the intermediate parent,
 * because that happens before it execs myapp.
 *
 * It's unclear why dbus-daemon needs to fork, but we explicitly tell it to
 * for some reason, then wait for it. If we left it undefined, a forking
 * dbus-daemon would get the parent process reparented to init and reaped
 * when the intermediate parent terminated, and a non-forking dbus-daemon
 * would get reparented to init and carry on there.
 *
 * myapp is exec'd by the process that initially ran main() so that it's
 * the shell's child, so the shell knows how to do job control and stuff.
 * This is desirable for the "dbus-launch an application" use-case, less so
 * for the "dbus-launch a test suite in an isolated session" use-case.
 */

I think we should have a separate tool - call it dbus-run-session or something - which forks like this:

shell
  \- dbus-run-session myapp
     \- (fork and exec) dbus-daemon --no-fork --print-address --session
     \- (then fork and exec) myapp

and does not exit until myapp does, at which point it kills its dbus-daemon.

This doesn't have the same semantics as dbus-launch as regards job control (^Z would stop dbus-run-in-session, the dbus-daemon, *and* myapp), but would be the appropriate tool for running a regression test (or whatever) under a temporary dbus-daemon.

telepathy-glib contains a shell implementation of this:

http://cgit.freedesktop.org/telepathy/telepathy-glib/tree/tools/with-session-bus.sh

Created attachment 49052 [details] [review]
add dbus-run-session

Created attachment 49053 [details] [review]
Recomend dbus-run-session over dbus-launch for starting  text-mode sessions

We can't reliably detect the lifetime of a text login, but we can detect the lifetime of a child process. It follows that the session ought to be our child process.

Review of attachment 49052 [details] [review]:

Revisiting my own patch:

::: tools/dbus-run-session.c
@@ +64,3 @@
+{
+  fprintf (stderr,
+      "dbus-run-session APP [APP_ARGUMENTS]\n"

Should be

    dbus-run-session [OPTIONS] [--] APP [APP_ARGUMENTS]

and document its options separately, I think. At the moment the only option is --config-file, but that will change.

@@ +200,3 @@
+
+  execl (DBUS_DAEMONDIR "/dbus-daemon",
+         DBUS_DAEMONDIR "/dbus-daemon",

Since this utility isn't run automatically, I'm tempted to say it should just search $PATH like everyone else, rather than hard-coding things. Doing that would make it easy to understand, at least.

There should also be a --dbus-daemon argument, so you can run:

    /opt/bin/dbus-run-session --dbus-daemon=/opt/bin/dbus-daemon /opt/bin/MyApp

if you want to be a control freak about it. Also very useful for regression tests: IMO we should replace all our uses of run-with-tmp-session-bus.sh (which uses dbus-launch) with something like

    TESTS_ENVIRONMENT = $(top_builddir)/tools/dbus-run-session$(EXEEXT) \
        --dbus-daemon=$(top_builddir)/bus/dbus-daemon$(EXEEXT) \
        --

which would run each test in an isolated dbus-daemon of its own, and is guaranteed not to leak the dbus-launch and dbus-daemon processes.

Created attachment 56723 [details] [review]
[1/3 v2] Add dbus-run-session

Created attachment 56724 [details] [review]
[2/3 v2] Recomend dbus-run-session over dbus-launch for starting  text-mode sessions

Created attachment 56725 [details] [review]
[3/3] massively simplify run-with-tmp-session-bus.sh by using  dbus-run-session

It turns out that if you don't second-guess the system by catching
SIGINT, the right things happen: it's received by every program in the
foreground process group, including dbus-run-session and dbus-daemon.
Neither of those catch SIGINT (unlike dbus-launch) so they'll exit
gracefully without the wrapper script needing to do anything special.

(In reply to comment #4)
> Revisiting my own patch:
...
> Should be
> 
>     dbus-run-session [OPTIONS] [--] APP [APP_ARGUMENTS]
> 
> and document its options separately, I think. At the moment the only option is
> --config-file, but that will change.

Done

> Since this utility isn't run automatically, I'm tempted to say it should just
> search $PATH like everyone else

Done

> There should also be a --dbus-daemon argument

Done

> Also very useful for regression
> tests: IMO we should replace all our uses of run-with-tmp-session-bus.sh
> (which uses dbus-launch) with something like
> 
>     TESTS_ENVIRONMENT = $(top_builddir)/tools/dbus-run-session$(EXEEXT) \
>         --dbus-daemon=$(top_builddir)/bus/dbus-daemon$(EXEEXT) \
>         --

I did the next best thing and made r-w-t-s-b.sh (which has other side-effects) call dbus-run-session. Further changes could be made to address Bug #44455.

As with Bug #39197, I believe these patches are ready; they only require review from some developer who knows what they're talking about. (As described in HACKING, this does not necessarily need to be a member of the D-Bus reviewers group, since the patches were written by a member of the reviewers group, namely me).

I'd really like to get this into 1.6. We've been recommending dbus-launch for this use for years, but it isn't actually very suitable (see Comment #1).

Telepathy projects run their tests under with-session-bus.sh, which is basically a less efficient dbus-run-session done in shell script (using temporary files instead of multiple pipes). I'd love to be able to get rid of it when D-Bus 1.6 becomes widespread - dbus-run-session does the job better and won't have to be copied into everyone's source tree.

I totally +1 the idea, and from my little dbus/unix competence it looks good. But I don't feel qualified enough to let you merge that ;-)

(In reply to comment #15)
> I'd really like to get this into 1.6 or even 1.4. It fixes a long-standing
> Debian bug.

This is not going to be in 1.6.0, I've given up on waiting for it.

My immediate reaction to reading the first patch was one of disappointment to see another half a getopt and another “read a line of text from an fd”...

(In reply to comment #12)
> My immediate reaction to reading the first patch was one of disappointment to
> see another half a getopt and another “read a line of text from an fd”...

We are indeed the knights who say NIH. As much as I'd love to just depend on GLib (or some other "make C less painful" library), I don't think it's likely to happen, particularly with dbus' bizarre licensing.

Do you approve of this tool in principle? Would you be more willing to r+ it if I added a tools-common.c with oom(), xmalloc(), read_line() and so on? I did part of that in a patch for Bug #34140, although that version was designed for use with the shared libdbus.

Regarding getopt, would you be more willing to r+ it if I imported GNU getopt_long from gnulib and used that ? (It's a libegg-style copy-paste library, so it has its own problems... but getopt_long is a GNUism.)

I'm not sure if I'm able to review the implementation competently, but the interface is exactly what I need and what I implemented in bash at https://github.com/mvidner/ruby-dbus/blob/v0.9.0/test/dbus-launch-simple

Comment on attachment 56723 [details] [review]
[1/3 v2] Add dbus-run-session

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

One minor bit, otherwise looks good.

::: tools/dbus-run-session.c
@@ +368,5 @@
> +    }
> +
> +  if (bus_pid == 0)
> +    {
> +      /* child */

We're leaking the read side of the pipe here into the child.  Probably doesn't matter really, but in other programs where we don't control both ends it's something to watch out for.  You could easily add

close (bus_address_pipe[PIPE_READ_END]);

here though.

Comment on attachment 56724 [details] [review]
[2/3 v2] Recomend dbus-run-session over dbus-launch for starting  text-mode sessions

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

Right, makes sense.

Comment on attachment 56725 [details] [review]
[3/3] massively simplify run-with-tmp-session-bus.sh by using  dbus-run-session

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

This looks a lot nicer for sure!

(In reply to comment #15)
> We're leaking the read side of the pipe here into the child.

No, exec_dbus_daemon() closes it before it exec()s the dbus-daemon.

(In reply to comment #15)
> Comment on attachment 56723 [details] [review]
> [1/3 v2] Add dbus-run-session
> 
> One minor bit, otherwise looks good.

In git for 1.7.4.

I also converted the man page into Docbook (mostly via doclifter), to catch up with the other documentation.

(In reply to comment #17)
> This looks a lot nicer for sure!

I reformatted this bit into Docbook but kept the wording you reviewed.

Created attachment 80397 [details] [review]
Export dbus_setenv() as a utility function

It's sufficiently portable that GLib has an equivalent, and I really
don't want to have to either open-code it in dbus-run-session or
link dbus-run-session statically. We have enough statically-linked
rubbish already.

Created attachment 80398 [details] [review]
dbus-run-session: remove various extra variables from  the environment

DBUS_SESSION_BUS_PID is not mandatory to set, but we should unset it
if present, since it points to a different session's bus. Likewise for
DBUS_SESSION_BUS_WINDOWID.

Similarly, if DBUS_STARTER_BUS_TYPE and DBUS_STARTER_ADDRESS
are set (as they would be under GNOME Terminal 3.8, see
<https://bugs.freedesktop.org/show_bug.cgi?id=63119>) then they
are likely to point to a different session's bus.

(In reply to comment #20)
> Export dbus_setenv() as a utility function

(In reply to comment #21)
> dbus-run-session: remove various extra variables from  the environment

Ping?

I'd really rather have this included before people start relying on dbus-run-session for regression tests.

Comment on attachment 80397 [details] [review]
Export dbus_setenv() as a utility function

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

BTW, see https://bugs.freedesktop.org/show_bug.cgi?id=65681 for the hazards of setenv.  Luckily I think dbus only has at most one internal thread for the SELinux policy reload notifications, but were we to use setenv() in more places we should be aware of it.

I'm fine with the patch given the rationale, it's not like we're exporting the mainloop or anything.

Comment on attachment 80398 [details] [review]
dbus-run-session: remove various extra variables from  the environment

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

Looks right to me.

Fixed in 1.7.4 (dbus-run-session is experimental) and 1.7.6 (works rather better).