Bug 39196 - dbus-launch can't be told to exit with its child
dbus-launch can't be told to exit with its child
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: core
unspecified
All All
: medium normal
Assigned To: Simon McVittie
John (J5) Palmieri
https://bugzilla.novell.com/show_bug....
review?
: patch
Depends on: 39197
Blocks: dbus-1.5
  Show dependency treegraph
 
Reported: 2011-07-13 08:24 UTC by Simon McVittie
Modified: 2013-08-23 10:47 UTC (History)
10 users (show)

See Also:


Attachments
add dbus-run-session (14.48 KB, patch)
2011-07-13 11:54 UTC, Simon McVittie
Details | Splinter Review
Recomend dbus-run-session over dbus-launch for starting text-mode sessions (1.81 KB, patch)
2011-07-13 11:55 UTC, Simon McVittie
Details | Splinter Review
[1/3 v2] Add dbus-run-session (16.29 KB, patch)
2012-02-07 12:26 UTC, Simon McVittie
Details | Splinter Review
[2/3 v2] Recomend dbus-run-session over dbus-launch for starting text-mode sessions (1.92 KB, patch)
2012-02-07 12:26 UTC, Simon McVittie
Details | Splinter Review
[3/3] massively simplify run-with-tmp-session-bus.sh by using dbus-run-session (2.94 KB, patch)
2012-02-07 12:27 UTC, Simon McVittie
Details | Splinter Review
Export dbus_setenv() as a utility function (8.76 KB, patch)
2013-06-06 12:22 UTC, Simon McVittie
Details | Splinter Review
dbus-run-session: remove various extra variables from the environment (3.39 KB, patch)
2013-06-06 12:22 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2011-07-13 08:24:55 UTC
On Bug #9884, Harri wrote:
> I am just starting dbus-launcher with "--exit-with-session xinit ...". I do not
> see what is happening inside, but dbus-launcher seems to start dbus-daemon, and
> then it starts xinit. It could terminate dbus-daemon when xinit terminates
> (watching both via wait(2) or sigaction(2) as usual), and then exit with
> xinit's exit value.
>
> Actually I don't see any reason why Dbus should work with XWindow only.
> Wouldn't you agree that Dbus could be used for all kinds of applications?

Havoc wrote:
> dbus-launch could terminate when the launched child does, but it doesn't do
> that now and isn't designed/documented to do that now, iirc. It probably is not
> a compatible change so would be a new feature like --exit-with-launched-child
Comment 1 Simon McVittie 2011-07-13 08:33:20 UTC
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
Comment 2 Simon McVittie 2011-07-13 11:54:15 UTC
Created attachment 49052 [details] [review]
add dbus-run-session
Comment 3 Simon McVittie 2011-07-13 11:55:21 UTC
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.
Comment 4 Simon McVittie 2011-10-14 04:37:50 UTC
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.
Comment 5 Simon McVittie 2012-02-07 12:26:09 UTC
Created attachment 56723 [details] [review]
[1/3 v2] Add dbus-run-session
Comment 6 Simon McVittie 2012-02-07 12:26:37 UTC
Created attachment 56724 [details] [review]
[2/3 v2] Recomend dbus-run-session over dbus-launch for starting  text-mode sessions
Comment 7 Simon McVittie 2012-02-07 12:27:21 UTC
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.
Comment 8 Simon McVittie 2012-02-07 12:29:28 UTC
(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.
Comment 9 Simon McVittie 2012-03-12 05:26:02 UTC
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.
Comment 10 Xavier Claessens 2012-03-26 06:33:43 UTC
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 ;-)
Comment 11 Simon McVittie 2012-06-05 03:56:19 UTC
(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.
Comment 12 Will Thompson 2012-06-15 06:32:06 UTC
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”...
Comment 13 Simon McVittie 2012-06-22 04:50:06 UTC
(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.)
Comment 14 Martin Vidner 2012-11-23 12:10:45 UTC
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 15 Colin Walters 2013-05-23 20:32:31 UTC
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 16 Colin Walters 2013-05-23 20:33:23 UTC
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 17 Colin Walters 2013-05-23 20:35:21 UTC
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!
Comment 18 Simon McVittie 2013-06-05 16:45:21 UTC
(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.
Comment 19 Simon McVittie 2013-06-05 17:02:43 UTC
(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.
Comment 20 Simon McVittie 2013-06-06 12:22:00 UTC
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.
Comment 21 Simon McVittie 2013-06-06 12:22:18 UTC
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.
Comment 22 Simon McVittie 2013-08-22 17:10:00 UTC
(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 23 Colin Walters 2013-08-22 18:04:38 UTC
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 24 Colin Walters 2013-08-22 18:12:55 UTC
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.
Comment 25 Simon McVittie 2013-08-23 10:47:20 UTC
Fixed in 1.7.4 (dbus-run-session is experimental) and 1.7.6 (works rather better).