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).
Created attachment 49046 [details] [review] [PATCH 1/5] document how the various processes in dbus-launch interact
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).
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.
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.
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.
Thoughts from the review cabal? Thoughts about whether this would be OK for 1.4?
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)?
(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!)
Created attachment 56718 [details] [review] [PATCH 3/5 v2] dbus-launch: add --exit-with-x11 option Fixed in response to Cosimo's review.
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.
Created attachment 56720 [details] [review] [3/5 v2] dbus-launch: add --exit-with-x11 option Rebased onto man page improvements in master.
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.
Created attachment 56722 [details] [review] [5/5 v2] Document that dbus-launch is not dbus-run-session Also with correct \- usage.
See also Bug #39196, which fixes the other half of this bug by introducing a new tool for command-line users and regression tests.
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.
(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 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 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 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 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 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.
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 :-)
Turns out this doesn't actually work.
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.
(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 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); }
(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.
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.
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.
(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?
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.
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 on attachment 80363 [details] [review] dbus-launch: add --exit-with-x11 option Review of attachment 80363 [details] [review]: ----------------------------------------------------------------- Looks good.
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.
Thanks, I'll merge these soon.
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.