GNOME Bugzilla – Bug 747941
try XDG_RUNTIME_DIR/bus before falling back to X11 autolaunch (dbus-launch)
Last modified: 2015-06-09 17:30:29 UTC
In <https://bugs.freedesktop.org/show_bug.cgi?id=61301> (merged for dbus 1.9.14), I made libdbus try XDG_RUNTIME_DIR/bus before falling back to X11 autolaunch (dbus-launch). This makes it do the right thing on systems where the dbus.socket/dbus.service user services in dbus 1.9.14 are enabled, and only costs one syscall on systems that still use the traditional one-bus-per-login-session model. I'd like to make GDBus do the same, so that in however many years' time, we can finally stop setting DBUS_SESSION_BUS_ADDRESS.
Created attachment 301672 [details] [review] [1/3] g_dbus_address_connect: specifically use dbus-launch for autolaunch: This only alters what happens if we specifically connect to "autolaunch:", for instance via "DBUS_SESSION_BUS_ADDRESS=autolaunch:". We will still potentially try other platform-specific things if DBUS_SESSION_BUS_ADDRESS is unset. There are currently no other platform-specific things, so there is no practical difference yet, but I'm about to add a more-preferred fallback path before autolaunch. This matches libdbus' behaviour and the D-Bus Specification, in which the autolaunch: transport specifically means X11 autolaunch (as implemented by "dbus-launch --autolaunch") on Unix, or a shared-memory-based protocol on Windows. Other platform-specific transports or default/fallback modes, including launchd on Mac OS X and XDG_RUNTIME_DIR/bus on Unix, are not part of "autolaunch:". It's rather unfortunate that the same name means two different platform-specific mechanisms, specific to different platforms - if they were added today I'd call them x11: and windows-shm: or something - but it's been like this since 2007 so it's too late now.
Created attachment 301673 [details] [review] [2/3] GDBus: try XDG_RUNTIME_DIR/bus before resorting to dbus-launch This is the right thing to do for the "a session is a user-session" model implemented in dbus 1.9.14, which is described in <http://lists.freedesktop.org/archives/dbus/2015-January/016522.html>. It also resembles sd-bus' behaviour, although sd-bus will only try kdbus and XDG_RUNTIME_DIR/bus, and never runs dbus-launch. On systems following the more traditional "a session is a login-session" model, X_R_D/bus won't exist, so it is harmless to check for it before falling back to X11 autolaunching. Again, this matches the behaviour of current libdbus and sd-bus versions. Now that we do this, g_test_dbus_unset() needs to clear XDG_RUNTIME_DIR as well as everything else.
Created attachment 301674 [details] [review] [3/3] Regression test for falling back to autolaunch: and XDG_RUNTIME_DIR/bus
Review of attachment 301673 [details] [review]: Looks good to me overall, from a code quality POV. ::: gio/gdbusaddress.c @@ +1481,3 @@ + gchar *ret; + + ret = get_session_address_xdg (); Some of the reasoning from the commit message could be commented here.
Review of attachment 301674 [details] [review]: ::: gio/tests/gdbus-unix-addresses.c @@ +39,3 @@ + + g_assert_no_error (error); + g_assert_true (addr != NULL); g_assert_nonnull (addr)? @@ +40,3 @@ + g_assert_no_error (error); + g_assert_true (addr != NULL); + g_print ("%s\n", addr); Leaks addr here. @@ +59,3 @@ + g_assert_true (G_IS_SOCKET (mock_bus)); + + if (g_mkdtemp_full (tmpdir, 0700) != tmpdir) Comparing against tmpdir is a bit confusing. How about (g_mkdtemp_full() == NULL)?
(In reply to Philip Withnall from comment #4) > Some of the reasoning from the commit message could be commented here. Fair point. (In reply to Philip Withnall from comment #5) > g_assert_nonnull (addr)? I've obviously got too used to sticking to old-GLib APIs. Fixed. > + g_print ("%s\n", addr); > > Leaks addr here. Good catch; fixed. > + if (g_mkdtemp_full (tmpdir, 0700) != tmpdir) > > Comparing against tmpdir is a bit confusing. How about (g_mkdtemp_full() == > NULL)? Sure. I also added a comment that it edits tmpdir in-place.
(In reply to Simon McVittie from comment #1) > Created attachment 301672 [details] [review] > [1/3] g_dbus_address_connect: specifically use dbus-launch for autolaunch: No comments on / complaints about this one?
Created attachment 301980 [details] [review] [2/3 v2] GDBus: try XDG_RUNTIME_DIR/bus before resorting to dbus-launch (same long commit message as before) --- v2: more comments; no functional changes.
Created attachment 301981 [details] [review] [3/3 v2] Regression test for falling back to autolaunch: and XDG_RUNTIME_DIR/bus --- v2: fix leaked address; clearer temporary directory creation; use g_assert_nonnull()
Review of attachment 301980 [details] [review]: ++ from me.
Review of attachment 301672 [details] [review]: (In reply to Simon McVittie from comment #7) > (In reply to Simon McVittie from comment #1) > > Created attachment 301672 [details] [review] [review] > > [1/3] g_dbus_address_connect: specifically use dbus-launch for autolaunch: > > No comments on / complaints about this one? None; sorry, I should have marked it as reviewed.
Review of attachment 301981 [details] [review]: ++ from me.
Any objection to this from GDBus maintainers, or may I commit these as reviewed-by: Philip?
Created attachment 302459 [details] [review] [3/3 v3] Regression test for falling back to autolaunch: and XDG_RUNTIME_DIR/bus --- The only change is to set a dummy DISPLAY in set_up_mock_dbus_launch(), so that on systems with the change that I proposed in Bug #723506 (this currently means Debian and its derivatives), the test will not fail with: # Start of gdbus tests # child process (/gdbus/x11-autolaunch [22203]) stdout: # child process (/gdbus/x11-autolaunch [22203]) stderr: **\nGLib-GIO:ERROR:/home/user/glib2.0-2.44.0/./gio/tests/gdbus-unix-addresses.c:40:print_address: assertion failed (error == NULL): Cannot autolaunch D-Bus without X11 $DISPLAY (g-io-error-quark, 0)\n # GLib-GIO:ERROR:/home/user/glib2.0-2.44.0/./gio/tests/gdbus-unix-addresses.c:127:test_x11_autolaunch: stderr of child process (/gdbus/x11-autolaunch [22203]) contains invalid match: ?*
(In reply to Simon McVittie from comment #14) > the test will not fail with: > > # Start of gdbus tests > # child process (/gdbus/x11-autolaunch [22203]) stdout: > # child process (/gdbus/x11-autolaunch [22203]) stderr: > **\nGLib-GIO:ERROR:/home/user/glib2.0-2.44.0/./gio/tests/gdbus-unix- > addresses.c:40:print_address: assertion failed (error == NULL): Cannot > autolaunch D-Bus without X11 $DISPLAY (g-io-error-quark, 0)\n > # > GLib-GIO:ERROR:/home/user/glib2.0-2.44.0/./gio/tests/gdbus-unix-addresses.c: > 127:test_x11_autolaunch: stderr of child process (/gdbus/x11-autolaunch > [22203]) contains invalid match: ?* The lines starting "# child process" are added by my patch on Bug #748534; this failure was difficult to debug without that.
Review of attachment 302459 [details] [review]: Looks good to me.
(In reply to Simon McVittie from comment #13) > Any objection to this from GDBus maintainers, or may I commit these as > reviewed-by: Philip? It's been a few weeks, so I'm going to take that as "no objection" and commit the patches. Fixed in git for 2.45.3.
Comment on attachment 301672 [details] [review] [1/3] g_dbus_address_connect: specifically use dbus-launch for autolaunch: 0d3f56e31c9099653539fa2539e05b3ba336a441
Comment on attachment 301980 [details] [review] [2/3 v2] GDBus: try XDG_RUNTIME_DIR/bus before resorting to dbus-launch 32492c6ab0000c50564360c74acf069814d942d1
Comment on attachment 302459 [details] [review] [3/3 v3] Regression test for falling back to autolaunch: and XDG_RUNTIME_DIR/bus b701c3c60824fca4c0056a7a46c627fe2977257d