|
Description
Simon McVittie
2011-07-13 08:41:59 UTC
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.