Bug 39197 - dbus-launch consumes characters from stdin
Summary: dbus-launch consumes characters from stdin
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://bugzilla.novell.com/show_bug....
Whiteboard: review+, waiting for fd.o git
Keywords: patch
Depends on: 97014
Blocks: dbus-1.5 39196
  Show dependency treegraph
 
Reported: 2011-07-13 08:41 UTC by Simon McVittie
Modified: 2016-07-25 11:46 UTC (History)
11 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/5] document how the various processes in dbus-launch interact (2.54 KB, patch)
2011-07-13 09:37 UTC, Simon McVittie
Details | Splinter Review
[PATCH 2/5] dbus-launch: if using X to define the session lifetime, do not poll stdin (1.96 KB, patch)
2011-07-13 09:37 UTC, Simon McVittie
Details | Splinter Review
[PATCH 3/5] dbus-launch: add --exit-with-x11 option (5.32 KB, patch)
2011-07-13 09:38 UTC, Simon McVittie
Details | Splinter Review
[PATCH 4/5] dbus-launch: revise recommendations and put them in an EXAMPLES section (2.80 KB, patch)
2011-07-13 09:38 UTC, Simon McVittie
Details | Splinter Review
[PATCH 5/5] Document that dbus-launch is not dbus-run-as-session (1.16 KB, patch)
2011-07-13 09:38 UTC, Simon McVittie
Details | Splinter Review
[PATCH 3/5 v2] dbus-launch: add --exit-with-x11 option (5.43 KB, patch)
2012-02-07 12:20 UTC, Simon McVittie
Details | Splinter Review
[2/5 v2] dbus-launch: if using X to define the session lifetime, do not poll stdin (2.02 KB, patch)
2012-02-07 12:22 UTC, Simon McVittie
Details | Splinter Review
[3/5 v2] dbus-launch: add --exit-with-x11 option (5.43 KB, patch)
2012-02-07 12:22 UTC, Simon McVittie
Details | Splinter Review
[4/5 v2] dbus-launch: revise recommendations and put them in an EXAMPLES section (2.90 KB, patch)
2012-02-07 12:23 UTC, Simon McVittie
Details | Splinter Review
[5/5 v2] Document that dbus-launch is not dbus-run-session (1.21 KB, patch)
2012-02-07 12:24 UTC, Simon McVittie
Details | Splinter Review
dbus-launch: read machine UUID before x11_init if --exit-with-x11 is used (1.41 KB, patch)
2012-06-25 13:11 UTC, Simon McVittie
Details | Splinter Review
dbus-launch: add --exit-with-x11 option (5.89 KB, patch)
2013-06-05 17:36 UTC, Simon McVittie
Details | Splinter Review
dbus-launch: read machine UUID before x11_init if --exit-with-x11 is used (1.46 KB, patch)
2013-06-05 17:37 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:41:59 UTC
On Bug #9884, Harri wrote:
> Since --exit-with-session polls stdin it keeps your terminal busy. This has the
> "nice" effect that if you run the Xwindow client script as a background
> process, dbus-launch eats up every second char you type on the terminal.

(This is also <http://bugs.debian.org/453755>).

This is a consequence of the multiple semantics of --exit-with-session. --exit-with-session currently means:

- if DISPLAY points to an X server, exit when the X session ends
- if stdin is a terminal, exit when end-of-file is reached
- if both are true, exit when one of them happens, whichever is first
- if neither is true, fail

I assert that these are not useful semantics: in most situations, you either want dbus-daemon to exit at the end of the X session, or when a child process exits (Bug #39196).

dbus-launch also "supports" exiting when EOF is reached on a terminal, but not in a particularly useful way, since it consumes characters from that terminal.

We should provide a way for dbus-launch to not poll the terminal, while still exiting with the X session (and make --autolaunch imply that new mechanism instead of --exit-with-session).
Comment 1 Simon McVittie 2011-07-13 09:37:24 UTC
Created attachment 49046 [details] [review]
[PATCH 1/5] document how the various processes in dbus-launch  interact
Comment 2 Simon McVittie 2011-07-13 09:37:50 UTC
Created attachment 49047 [details] [review]
[PATCH 2/5] dbus-launch: if using X to define the session lifetime,  do not poll stdin

dbus-launch --exit-with-session attempts to scope the session length
to various things:

- if DISPLAY points to an X server, exit when the X session ends
- if stdin is a terminal, exit when end-of-file is reached
- if both are true, exit when one of them happens, whichever is first
- if neither is true, fail

These are not particularly useful semantics: if the session is scoped to
the X session, then the terminal from which dbus-launch was launched
is irrelevant. This also causes practical problems when dbus-launch
consumes characters from the terminal from which it happens to have
been launched (some display managers, like slim and nodm, run users' X
sessions with stdin pointing to the terminal from which the init daemon
happens to have started the display manager during boot, usually tty1
on Linux).
Comment 3 Simon McVittie 2011-07-13 09:38:09 UTC
Created attachment 49048 [details] [review]
[PATCH 3/5] dbus-launch: add --exit-with-x11 option

This is more suitable for distributions' Xsession scripts: it verifies
that X is already available, and so never results in an attempt to poll
stdin.
Comment 4 Simon McVittie 2011-07-13 09:38:31 UTC
Created attachment 49049 [details] [review]
[PATCH 4/5] dbus-launch: revise recommendations and put them in an  EXAMPLES section

The first thing we should talk about is how to get a D-Bus session in
your X session - that's the common case.

Secondarily, we can tell command-line addicts how to have a D-Bus session.
Do not recommend --exit-with-session here, since that polls (and reads
from) stdin, which is harmful to precisely those command-line users!
Until we have some better tool, the best we can do here is note that
the dbus-daemon is not automatically terminated.
Comment 5 Simon McVittie 2011-07-13 09:38:51 UTC
Created attachment 49050 [details] [review]
[PATCH 5/5] Document that dbus-launch is not dbus-run-as-session

Architectural assumptions inside dbus-launch mean that it is unsuitable
for use in contexts where a particular process's lifetime defines the
session, unless there is an out-of-band mechanism (like the X server)
which can signal the end of the session.
Comment 6 Simon McVittie 2011-07-13 09:41:19 UTC
Thoughts from the review cabal? Thoughts about whether this would be OK for 1.4?
Comment 7 Cosimo Alfarano 2011-10-13 09:27:48 UTC
Review of attachment 49047 [details] [review]:

::: tools/dbus-launch.c
@@ +493,3 @@
     tty_fd = -1;
 
+  if (x_fd < 0)

Maybe I'm getting what you're doing wrong, but shouldn't it be (x_fd > 0) to check that it's a fd (AKA a display is set)?
Comment 8 Simon McVittie 2011-10-13 10:16:07 UTC
(In reply to comment #7)
> Maybe I'm getting what you're doing wrong, but shouldn't it be (x_fd > 0) to
> check that it's a fd (AKA a display is set)?

>= really, but yes, I had the check backwards.

(0 is a valid fd - although if your connection to X is also your stdin, dbus-launch misbehaving is probably the least of your problems!)
Comment 9 Simon McVittie 2012-02-07 12:20:16 UTC
Created attachment 56718 [details] [review]
[PATCH 3/5 v2] dbus-launch: add --exit-with-x11 option

Fixed in response to Cosimo's review.
Comment 10 Simon McVittie 2012-02-07 12:22:03 UTC
Created attachment 56719 [details] [review]
[2/5 v2] dbus-launch: if using X to define the session lifetime, do not poll stdin

Sorry, *this* one is fixed according to Cosimo's review.
Comment 11 Simon McVittie 2012-02-07 12:22:43 UTC
Created attachment 56720 [details] [review]
[3/5 v2] dbus-launch: add --exit-with-x11 option

Rebased onto man page improvements in master.
Comment 12 Simon McVittie 2012-02-07 12:23:40 UTC
Created attachment 56721 [details] [review]
[4/5 v2] dbus-launch: revise recommendations and put them in an EXAMPLES section

Now with man page syntax fixed to use \- correctly.
Comment 13 Simon McVittie 2012-02-07 12:24:12 UTC
Created attachment 56722 [details] [review]
[5/5 v2] Document that dbus-launch is not dbus-run-session

Also with correct \- usage.
Comment 14 Simon McVittie 2012-02-07 12:25:27 UTC
See also Bug #39196, which fixes the other half of this bug by introducing a new tool for command-line users and regression tests.
Comment 15 Simon McVittie 2012-03-12 05:27:48 UTC
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'm looking at you, Michael :-P)

I'd really like to get this into 1.6 or even 1.4. It fixes a long-standing Debian bug.
Comment 16 Simon McVittie 2012-06-05 03:56:03 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 17 Will Thompson 2012-06-07 08:58:03 UTC
Comment on attachment 49046 [details] [review]
[PATCH 1/5] document how the various processes in dbus-launch  interact

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

Looks good.
Comment 18 Will Thompson 2012-06-07 08:58:52 UTC
Comment on attachment 56719 [details] [review]
[2/5 v2] dbus-launch: if using X to define the session lifetime, do not poll stdin

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

Looks reasonable to me.
Comment 19 Will Thompson 2012-06-07 09:02:18 UTC
Comment on attachment 56720 [details] [review]
[3/5 v2] dbus-launch: add --exit-with-x11 option

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

There's a little bit of spooky-action-at-a-distance in checking x11_init() and then just passing TRUE to babysit() on the assumption that it will behave as the previous patch made it behave, but I guess that's fine. This looks like a more sensible command-line option to have.
Comment 20 Will Thompson 2012-06-07 09:04:31 UTC
Comment on attachment 56721 [details] [review]
[4/5 v2] dbus-launch: revise recommendations and put them in an EXAMPLES section

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

::: doc/dbus-launch.1
@@ +90,3 @@
>  .fi
> +Note that in this case, dbus-launch will exit, and dbus-daemon will not be
> +terminated automatically on logout.

It's a little unsatisfactory to have to say this, but I guess that's the point of 39196.
Comment 21 Will Thompson 2012-06-07 09:05:02 UTC
Comment on attachment 56722 [details] [review]
[5/5 v2] Document that dbus-launch is not dbus-run-session

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

Saddening, but okay.
Comment 22 Simon McVittie 2012-06-15 05:39:32 UTC
Thanks, fixed in git for 1.6.2, 1.7.0.

(In reply to comment #20)
> It's a little unsatisfactory to have to say this, but I guess that's the point
> of 39196.

Yes. Reviews welcome :-)
Comment 23 Simon McVittie 2012-06-25 13:10:00 UTC
Turns out this doesn't actually work.
Comment 24 Simon McVittie 2012-06-25 13:11:16 UTC
Created attachment 63464 [details] [review]
dbus-launch: read machine UUID before x11_init if  --exit-with-x11 is used

The machine UUID is needed to set the X atoms. x11_init() assumes that
the machine UUID (global variable) has been set, either via
read_machine_uuid_if_needed() or save_machine_uuid().

This is pretty tangled, but to make The Right Thing happen
automatically, we'd need to redo dbus-launch in terms of DBusError.
For dbus-1.6 let's just provide the prerequisite.
Comment 25 Simon McVittie 2012-06-27 10:55:05 UTC
(In reply to comment #19)
> [3/5 v2] dbus-launch: add --exit-with-x11 option

For 1.6.2 I reverted this part, since without Attachment #63464 [details] it doesn't work correctly, and amended the docs to talk about --exit-with-session instead. The new option is probably more suitable for 1.7.x anyway.
Comment 26 Colin Walters 2013-05-23 21:23:38 UTC
Comment on attachment 63464 [details] [review]
dbus-launch: read machine UUID before x11_init if  --exit-with-x11 is used

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

It looks like the code here has changed in master?  There's a bit now which has:

      if (get_machine_uuid () == NULL)
        {
          fprintf (stderr, "Machine UUID not provided as arg to --autolaunch\n");
          exit (1);
        }
Comment 27 Simon McVittie 2013-06-05 17:35:53 UTC
(In reply to comment #26)
> It looks like the code here has changed in master?

Ah, sorry, that was because I reverted the addition of --exit-with-x11, because it didn't work without this fix.
Comment 28 Simon McVittie 2013-06-05 17:36:54 UTC
Created attachment 80363 [details] [review]
dbus-launch: add --exit-with-x11 option

This is more suitable for distributions' Xsession scripts: it verifies
that X is already available, and so never results in an attempt to poll
stdin.

Reviewed-by: Will Thompson <will.thompson@collabora.co.uk>
[reformatted for Docbook man page -smcv]

---

This was already reviewed, but I reverted it, because it turned out to be not quite right. Here it is again.
Comment 29 Simon McVittie 2013-06-05 17:37:24 UTC
Created attachment 80364 [details] [review]
dbus-launch: read machine UUID before x11_init if  --exit-with-x11 is used

The machine UUID is needed to set the X atoms. x11_init() assumes that
the machine UUID (global variable) has been set, either via
read_machine_uuid_if_needed() or save_machine_uuid().

This is pretty tangled, but to make The Right Thing happen
automatically, we'd need to redo dbus-launch in terms of DBusError.
For now let's just provide the prerequisite.

---

Can be squashed into the above if desired.
Comment 30 Simon McVittie 2013-08-22 17:10:45 UTC
(In reply to comment #28)
> dbus-launch: add --exit-with-x11 option

(In reply to comment #29)
> dbus-launch: read machine UUID before x11_init if  --exit-with-x11 is used

Ping?
Comment 31 Simon McVittie 2016-07-01 15:58:24 UTC
This has now been waiting for review for 3 years. If nobody reviews it in the near future (let's say the next month), I'm just going to land it.
Comment 32 Will Thompson 2016-07-01 16:54:26 UTC
Obviously take this with a pinch of salt because it's been a long time since I thought about this stuff, but I've refreshed my memory and looked over this new patch, and both your justification and the code look okay to me.
Comment 33 Thiago Macieira 2016-07-06 16:45:14 UTC
Comment on attachment 80363 [details] [review]
dbus-launch: add --exit-with-x11 option

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

Looks good.
Comment 34 Thiago Macieira 2016-07-06 16:45:46 UTC
Comment on attachment 80364 [details] [review]
dbus-launch: read machine UUID before x11_init if  --exit-with-x11 is used

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

Looks good.
Comment 35 Simon McVittie 2016-07-06 19:06:24 UTC
Thanks, I'll merge these soon.
Comment 36 Simon McVittie 2016-07-25 11:46:03 UTC
Fixed in git for 1.11.4, thanks.


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.