|
Description
Simon McVittie
2013-02-22 19:04:04 UTC
Created attachment 75347 [details] [review] specification: discuss "listenable" and "connectable" addresses The --with-dbus-session-bus-connect-address configure option and the DBUS_SESSION_BUS_DEFAULT_ADDRESS CMake variable expect a connectable address, while the --with-dbus-session-bus-listen-address option and the DBUS_SESSION_BUS_LISTEN_ADDRESS variable expect a listenable address. DBUS_SYSTEM_BUS_DEFAULT_ADDRESS currently has to be an address that is simultaneously listenable and connectable. --- This is just general documentation really, and I'd like to merge it regardless of whether people think xdg-runtime: is a good idea. Created attachment 75348 [details] [review] Move _dbus_string_get_dirname into the shared library DBusServer is about to need it. Created attachment 75349 [details] [review] _dbus_check_dir_is_private_to_user: check that we own it --- This is a bugfix, really. In practice it's never going to be owned by anyone else, but the sanity check seems good. Created attachment 75350 [details] [review] DBusServer: allow "owning" the directory containing the socket Created attachment 75351 [details] [review] Add xdg-runtime: transport This is basically the same as substituting the environment variable into "unix:path=$XDG_RUNTIME_DIR/dbus_user_bus_socket", but doesn't require escaping the XDG_RUNTIME_DIR if it contains special characters such as ':', and the XDG_RUNTIME_DIR can't get out of sync with the DBUS_SESSION_BUS_ADDRESS this way. Created attachment 75352 [details] [review] On Unix, try xdg-runtime: before autolaunch: by default This means that if DBUS_SESSION_BUS_ADDRESS is not set, but XDG_RUNTIME_DIR is, and $XDG_RUNTIME_DIR/dbus_user_bus_socket exists and can be connected to, we will use it instead of X11 autolaunch. Attachment #75347 [details], Attachment #75349 [details] are applicable even if xdg-runtime: is not added. See: http://lists.freedesktop.org/archives/systemd-devel/2013-August/012517.html So...my patch basically just adds a new default if DBUS_SESSION_BUS_ADDRESS is unset, whereas you're adding a new transport, then also defaulting to it. Is there a use case for explicitly specifying the user bus? > my patch basically just adds a new default if DBUS_SESSION_BUS_ADDRESS is unset
I thought it seemed better to ensure that there was always a way to replicate the behaviour of "DBUS_SESSION_BUS_ADDRESS is unset" by setting it to a non-empty value, rather than having another special case - explicit is better than implicit.
(Analogously, we have the "autolaunch:" transport, instead of having *that* be a special case when DBUS_SESSION_BUS_ADDRESS is unset.)
Any review on the earlier patches here, even if you don't like the new transport? Created attachment 89097 [details] [review] Colin's WiP patch for comparison --- If applied, this would replace Attachment #75351 [details], Attachment #75352 [details], and Attachment #75344 [details] from Bug #61301. Comment on attachment 89097 [details] [review] Colin's WiP patch for comparison Review of attachment 89097 [details] [review]: ----------------------------------------------------------------- Other things from my patches which I want if we go this route: regression test, documentation in the spec. ::: bus/Makefile.am @@ +219,5 @@ > ln -fs ../dbus.socket $(DESTDIR)$(systemdsystemunitdir)/sockets.target.wants/dbus.socket > $(mkinstalldirs) $(DESTDIR)$(systemdsystemunitdir)/multi-user.target.wants > ln -fs ../dbus.service $(DESTDIR)$(systemdsystemunitdir)/multi-user.target.wants/dbus.service > + $(mkinstalldirs) $(DESTDIR)$(systemduserunitdir)/sockets.target.wants > + ln -fs ../dbus.socket $(DESTDIR)$(systemduserunitdir)/sockets.target.wants/dbus.socket This part is missing from my Attachment #75344 [details], FWIW; please review on the assumption that I'll add it. @@ +283,5 @@ > dbus.socket > + > +systemduserunit_DATA = \ > + user-units/dbus.service \ > + user-units/dbus.socket I used a separate HAVE_USER_SYSTEMD so you can configure --without-systemduserunitdir if desired. ::: bus/user-units/dbus.service.in @@ +1,3 @@ > +[Unit] > +Description=D-Bus User Bus > +Documentation=man:dbus-daemon(1) Adding that line is an improvement over what I had; please review on the assumption that I'll add it. ::: bus/user-units/dbus.socket.in @@ +1,3 @@ > +[Unit] > +Description=D-Bus User Bus Socket > +Before=sockets.target Is this necessary/desirable? The system version doesn't have it. @@ +2,5 @@ > +Description=D-Bus User Bus Socket > +Before=sockets.target > + > +[Socket] > +ListenStream=%t/dbus/user_bus_socket I added +[Install] +WantedBy=sockets.target +Also=dbus.service which I think is probably also desirable? ::: dbus/dbus-sysdeps-unix.c @@ +3708,4 @@ > *supported = TRUE; > return _dbus_lookup_session_address_launchd (address, error); > #else > + const char *runtime_dir = _dbus_getenv ("XDG_RUNTIME_DIR"); I think I'd still prefer having this be a transport, so you can, e.g., set DBUS_SESSION_BUS_ADDRESS="xdg-runtime:" and it will definitely not do autolaunch... In my patches you can also configure a dbus-daemon to listen on "xdg-runtime:" if desired, which seems like a win. @@ +3718,5 @@ > + if (!_dbus_string_init (&user_bus_path)) > + return FALSE; > + if (!_dbus_string_append_printf (&user_bus_path, "%s/dbus/user_bus_socket", runtime_dir)) > + goto out; > + if (lstat (_dbus_string_get_const_data (&user_bus_path), &stbuf) != -1) Shouldn't this check that it is, in fact, a socket before deciding to use it? @@ +3723,5 @@ > + { > + if (!_dbus_string_append (address, "unix:path=")) > + goto out; > + if (!_dbus_string_append (address, _dbus_string_get_const_data (&user_bus_path))) > + goto out; If the user_bus_path contains special characters (which it might, because it comes from a user-controlled XDG_RUNTIME_DIR) then this will result in a syntactically invalid address. It should be escaped. Part of the reason I used a separate transport was to make that irrelevant. Comment on attachment 75347 [details] [review] specification: discuss "listenable" and "connectable" addresses Review of attachment 75347 [details] [review]: ----------------------------------------------------------------- Patch looks good, however, there is a typo in commit msg. the DBUS_SESSION_BUS_DEFAULT_ADDRESS CMake variable expect a connectable address should be the DBUS_SESSION_BUS_CONNECT_ADDRESS CMake variable expect a connectable address Comment on attachment 75350 [details] [review] DBusServer: allow "owning" the directory containing the socket Review of attachment 75350 [details] [review]: ----------------------------------------------------------------- I'm not sure if it's worth to introduce codes to handle parent directory giving that the parent directory (dbus) is under XDG_RUNTIME_DIR, which in turn is a temporary directory, only exists as long as a login user. Isn't the same as before the socket may exists in some permanent place. ::: dbus/dbus-server-unix.c @@ +231,4 @@ > * > * @param path the path for the domain socket. > * @param abstract #TRUE to use abstract socket namespace > + * @param own_parent if #TRUE, create the directory containing @path and try to unlink it on disconnection I think @path should be @p path here. (In reply to comment #7) > Attachment #75347 [details], Attachment #75349 [details] are applicable even > if xdg-runtime: is not added. Yes, these two looks good to me. Comment on attachment 75351 [details] [review] Add xdg-runtime: transport Review of attachment 75351 [details] [review]: ----------------------------------------------------------------- dbus_user_bus_socket should be dbus/user_bus_socket in commit msg. ::: dbus/dbus-server-unix.c @@ +2,4 @@ > /* dbus-server-unix.c Server implementation for Unix network protocols. > * > * Copyright (C) 2002, 2003, 2004 Red Hat Inc. > + * Copyright © 2013 Intel Corporation I'm doubting you're now working for Intel? ::: test/loopback.c @@ +269,5 @@ > g_test_add ("/message/unix", Fixture, "unix:tmpdir=/tmp", setup, > test_message, teardown); > + > + g_test_add ("/connect/xdg-runtime", Fixture, "xdg-runtime:", > + setup_xdg_runtime, test_message, teardown_xdg_runtime); Should be surrounded by #ifdef DBUS_UNIX ... #endif /* DBUS_UNIX */ Comment on attachment 75352 [details] [review] On Unix, try xdg-runtime: before autolaunch: by default Review of attachment 75352 [details] [review]: ----------------------------------------------------------------- dbus_user_bus_socket should be dbus/user_bus_socket in commit msg I think. (In reply to comment #12) > Comment on attachment 89097 [details] [review] [review] > Colin's WiP patch for comparison > > Review of attachment 89097 [details] [review] [review]: > ----------------------------------------------------------------- > > Other things from my patches which I want if we go this route: regression > test, documentation in the spec. > > ::: bus/Makefile.am > @@ +219,5 @@ > > ln -fs ../dbus.socket $(DESTDIR)$(systemdsystemunitdir)/sockets.target.wants/dbus.socket > > $(mkinstalldirs) $(DESTDIR)$(systemdsystemunitdir)/multi-user.target.wants > > ln -fs ../dbus.service $(DESTDIR)$(systemdsystemunitdir)/multi-user.target.wants/dbus.service > > + $(mkinstalldirs) $(DESTDIR)$(systemduserunitdir)/sockets.target.wants > > + ln -fs ../dbus.socket $(DESTDIR)$(systemduserunitdir)/sockets.target.wants/dbus.socket > > This part is missing from my Attachment #75344 [details], FWIW; please > review on the assumption that I'll add it. Good to know, I did add such a comment there. > > @@ +283,5 @@ > > dbus.socket > > + > > +systemduserunit_DATA = \ > > + user-units/dbus.service \ > > + user-units/dbus.socket > > I used a separate HAVE_USER_SYSTEMD so you can configure > --without-systemduserunitdir if desired. > > ::: bus/user-units/dbus.service.in > @@ +1,3 @@ > > +[Unit] > > +Description=D-Bus User Bus > > +Documentation=man:dbus-daemon(1) > > Adding that line is an improvement over what I had; please review on the > assumption that I'll add it. > > ::: bus/user-units/dbus.socket.in > @@ +1,3 @@ > > +[Unit] > > +Description=D-Bus User Bus Socket > > +Before=sockets.target > > Is this necessary/desirable? The system version doesn't have it. Negative from me, a unit installed into a target always before the target. So it's redundant. > > @@ +2,5 @@ > > +Description=D-Bus User Bus Socket > > +Before=sockets.target > > + > > +[Socket] > > +ListenStream=%t/dbus/user_bus_socket > > I added > > +[Install] > +WantedBy=sockets.target > +Also=dbus.service > > which I think is probably also desirable? Agree. > > ::: dbus/dbus-sysdeps-unix.c > @@ +3708,4 @@ > > *supported = TRUE; > > return _dbus_lookup_session_address_launchd (address, error); > > #else > > + const char *runtime_dir = _dbus_getenv ("XDG_RUNTIME_DIR"); > > I think I'd still prefer having this be a transport, so you can, e.g., set > DBUS_SESSION_BUS_ADDRESS="xdg-runtime:" and it will definitely not do > autolaunch... > > In my patches you can also configure a dbus-daemon to listen on > "xdg-runtime:" if desired, which seems like a win. > > @@ +3718,5 @@ > > + if (!_dbus_string_init (&user_bus_path)) > > + return FALSE; > > + if (!_dbus_string_append_printf (&user_bus_path, "%s/dbus/user_bus_socket", runtime_dir)) > > + goto out; > > + if (lstat (_dbus_string_get_const_data (&user_bus_path), &stbuf) != -1) > > Shouldn't this check that it is, in fact, a socket before deciding to use it? > > @@ +3723,5 @@ > > + { > > + if (!_dbus_string_append (address, "unix:path=")) > > + goto out; > > + if (!_dbus_string_append (address, _dbus_string_get_const_data (&user_bus_path))) > > + goto out; > > If the user_bus_path contains special characters (which it might, because it > comes from a user-controlled XDG_RUNTIME_DIR) then this will result in a > syntactically invalid address. It should be escaped. Part of the reason I > used a separate transport was to make that irrelevant. I'm not quite understand you about "because it comes from a user-controlled XDG_RUNTIME_DIR", does user can set XDG_RUNTIME_DIR in both cases if he want? (In reply to comment #14) > I'm not sure if it's worth to introduce codes to handle parent directory > giving that the parent directory (dbus) is under XDG_RUNTIME_DIR, which in > turn is a temporary directory, only exists as long as a login user. Isn't > the same as before the socket may exists in some permanent place. XDG_RUNTIME_DIR *as set by systemd-logind* is in a tmpfs, but the XDG base directory specification doesn't guarantee that. I can see your point, though; perhaps this isn't worth it. If anything non-systemd creates an XDG_RUNTIME_DIR in a persistent location, then it will have to clean it up eventually, and might as well clean up dbus-daemon's dbus subdirectory at the same time. (In reply to comment #16) > > * Copyright (C) 2002, 2003, 2004 Red Hat Inc. > > + * Copyright © 2013 Intel Corporation > > I'm doubting you're now working for Intel? I don't, but Intel is the copyright holder for the code added by that patch. (When I revise the patch, it will also be © 2013 Collabora.) > > + g_test_add ("/connect/xdg-runtime", Fixture, "xdg-runtime:", > > + setup_xdg_runtime, test_message, teardown_xdg_runtime); > > Should be surrounded by #ifdef DBUS_UNIX ... #endif /* DBUS_UNIX */ Yes, well spotted. (In reply to comment #18) > > If the user_bus_path contains special characters (which it might, because it > > comes from a user-controlled XDG_RUNTIME_DIR) then this will result in a > > syntactically invalid address. It should be escaped. Part of the reason I > > used a separate transport was to make that irrelevant. > > I'm not quite understand you about "because it comes from a user-controlled > XDG_RUNTIME_DIR", does user can set XDG_RUNTIME_DIR in both cases if he want? The session dbus-daemon is run with its environment variables controlled by the user. XDG_RUNTIME_DIR in that environment might have been set by systemd-logind, but it might equally well have been set by something else on an OS that does not use systemd, or it might have been set by a shell script to run regression tests in an isolated environment. As a concrete example of something that will fail, if you do something like this, for regression tests or something: export XDG_RUNTIME_DIR=${top_builddir}/tmp/XDG_RUNTIME_DIR and $top_builddir is in a directory containing special characters, like this: /home/jörg/my-c++-code then Colin's patch would fail, because the "ö" and "+" have to be escaped in order to form a valid D-Bus address. (In reply to comment #19) > > I'm not quite understand you about "because it comes from a user-controlled > > XDG_RUNTIME_DIR", does user can set XDG_RUNTIME_DIR in both cases if he want? ... > export XDG_RUNTIME_DIR=${top_builddir}/tmp/XDG_RUNTIME_DIR > /home/jörg/my-c++-code The reason that this isn't a problem in my implementation is that my implementation looks up XDG_RUNTIME_DIR, composes the socket path as a raw filesystem path and opens the socket without ever needing to substitute that path into a unix:path=xxx address, so escaping is not relevant. Looking at its implementation, _dbus_append_address_from_socket() will still need to be changed to escape the path correctly in order to work in that situation (Bug #46013). (In reply to comment #20) > (In reply to comment #19) > > > I'm not quite understand you about "because it comes from a user-controlled > > > XDG_RUNTIME_DIR", does user can set XDG_RUNTIME_DIR in both cases if he want? > ... > > export XDG_RUNTIME_DIR=${top_builddir}/tmp/XDG_RUNTIME_DIR > > /home/jörg/my-c++-code > > The reason that this isn't a problem in my implementation is that my > implementation looks up XDG_RUNTIME_DIR, composes the socket path as a raw > filesystem path and opens the socket without ever needing to substitute that > path into a unix:path=xxx address, so escaping is not relevant. > OK, got you. the key point is in Colin's patch, the XDG_RUNTIME_DIR will be append to unix:path= and so will be parsed dbus_parse_address. But in your patch, XDG_RUNTIME_DIR as a transport method. > Looking at its implementation, _dbus_append_address_from_socket() will still > need to be changed to escape the path correctly in order to work in that > situation (Bug #46013). I see that in libsystemd-bus, which take a kind of Colin's method but does escape the XDG_RUNTIME_DIR before append to unix:path=, something like:
dir = getenv ("XDG_RUNTIME_DIR");
escaped_dir = escape (dir);
asprintf (&address, "unix:path=%s/bus", escaped_dir);
Generally, the session bus path is mismatch, XDG_RUNTIME_DIR/bus (libsystemd-bus) and XDG_RUNTIME_DIR/dbus/user_bus_socket (in libdbus-1).
Hmm, I must say I find Colin's solution much nicer and would prefer not to intrdocue a new xdg-runtime: protocol definition... In libsystemd-bus we simply use XDG_RUNTIME_DIR as a fallback if $DBUS_SESSION_BUS is not set. I am pretty sure that is the simplest and most obvious solution... (In reply to comment #23) > Hmm, I must say I find Colin's solution much nicer and would prefer not to > intrdocue a new xdg-runtime: protocol definition... One advantage of xdg-runtime: is that it can decouple the point at which we implement this transport from the point at which distributions default to it. If we add xdg-runtime: but do not alter the default, then "early adopter" distributions that are willing to deal with potential system integration issues can do "./configure ... --with-dbus-session-bus-connect-address=xdg-runtime:,autolaunch:" to "opt in". We can change the default later, once there's implementation experience for XDG_RUNTIME_DIR. I've been asked to make a 1.8 stable branch of D-Bus by mid January; I don't think user sessions are going to be set in stone by that point, but if the code is in place and ready to go, distributions can enable it if desired. (In reply to comment #22) > Generally, the session bus path is mismatch, XDG_RUNTIME_DIR/bus > (libsystemd-bus) and XDG_RUNTIME_DIR/dbus/user_bus_socket (in libdbus-1). My patches use the path that all the third-party user-session experiments using systemd (e.g. user-session-units) seemed to have as a de facto standard at the time. If people now prefer XDG_RUNTIME_DIR/bus (or XDG_RUNTIME_DIR/dbus or any other sensible path), that's fine - eliminating the pointless subdirectory is a good thing. Comment on attachment 75351 [details] [review] Add xdg-runtime: transport Review of attachment 75351 [details] [review]: ----------------------------------------------------------------- ::: test/loopback.c @@ +115,5 @@ > +{ > + f->xdg_runtime_dir = g_dir_make_tmp ("dbus-test-loopback-XXXXXX", > + &f->error); > + g_assert_no_error (f->error); > + g_setenv ("XDG_RUNTIME_DIR", f->xdg_runtime_dir, TRUE); I know this is only test code, but see https://bugzilla.gnome.org/show_bug.cgi?id=659326 To do it safely you could use g_test_trap_subprocess(). At a high level I'm uncertain whether or not it makes sense to land patches like this until the "full picture" of user@.service is clear. That aside, coming back to this now, I find Simon's arguments for explicit xdg-runtime: make sense. (We could *also* do this automatically if the environment variable is not set of course) (In reply to comment #24) > (In reply to comment #23) > > Hmm, I must say I find Colin's solution much nicer and would prefer not to > > intrdocue a new xdg-runtime: protocol definition... > > One advantage of xdg-runtime: is that it can decouple the point at which we > implement this transport from the point at which distributions default to > it. If we add xdg-runtime: but do not alter the default, then "early > adopter" distributions that are willing to deal with potential system > integration issues can do "./configure ... > --with-dbus-session-bus-connect-address=xdg-runtime:,autolaunch:" to "opt > in". We can change the default later, once there's implementation experience > for XDG_RUNTIME_DIR. Well, I see no problem in simply always checking $XDG_RUNTIME_DIR/bus if $DBUS_SESSION_BUS_ADDRESS is not set. The worst thing that can happen is that the lib issues a connect() on a non-existing socket which will immediately fail and which will make it go on to the next address, but that's free, and all DEs set $DBUS_SESSION_BUS_ADDRESS anyway, so is never even done, in real-life. libsystemd-bus uses this logic to connect btw: http://cgit.freedesktop.org/systemd/systemd/tree/src/libsystemd-bus/sd-bus.c#n1096 i.e. we use $DBUS_SESSION_BUS_ADDRESS if it is set. If it isn't we use kdbus with a fallback to $XDG_RUNTIME_DIR/bus, and that's all. (we deliberately do not support the autostart stuff...). I think a similar logic in dbus1 (modulo the kdbus thing and the absence of autostart) would be a good fit for libdbus too? > (In reply to comment #22) > > Generally, the session bus path is mismatch, XDG_RUNTIME_DIR/bus > > (libsystemd-bus) and XDG_RUNTIME_DIR/dbus/user_bus_socket (in libdbus-1). > > My patches use the path that all the third-party user-session experiments > using systemd (e.g. user-session-units) seemed to have as a de facto > standard at the time. If people now prefer XDG_RUNTIME_DIR/bus (or > XDG_RUNTIME_DIR/dbus or any other sensible path), that's fine - eliminating > the pointless subdirectory is a good thing. In the systemd context we tried to avoid using the name "dbus" too much for the generic bits. We just refer to the "bus", without any further naming, since we want to ensure that there's no doubt that this is the one and only bus that matters on the OS, and not just one project among many. Inside of systemd we hence use "kdbus" to refer to the specific kernel implmentation of it and "dbus" (or sometimes "dbus1") for the good old userspace implementation, but in everything that covers both backends we just call it the "bus". Since behind $XDG_RUNTIME_DIR/bus there can be both the old and the new stuff I'd just call it "bus", and not "dbus". Also, I am kind of interested in removing the difference between $XDG_RUNTIME_DIR for user stuff and /run for system stuff in the naming. In systemd we have this specifier "%r" which resolves to the former when used in units in the user instance, and to the latter when used in units in the system instance. This would normally allow writing unit files which do the right thing on either. Of course this goal is lost on dbus already since the old system bus socket is explicitly called system_bus_socket, but for the newly introduced user socket, I'd prefer not to make the same mistake again, if you follow what I mean... Comment on attachment 75347 [details] [review] specification: discuss "listenable" and "connectable" addresses In 1.7.10 Comment on attachment 75349 [details] [review] _dbus_check_dir_is_private_to_user: check that we own it In 1.7.10 Comment on attachment 75350 [details] [review] DBusServer: allow "owning" the directory containing the socket Chengwei Yang convinced me that we don't really need this Comment on attachment 75348 [details] [review] Move _dbus_string_get_dirname into the shared library Unnecessary without the one I just marked as obsolete (In reply to comment #27) > Well, I see no problem in simply always checking $XDG_RUNTIME_DIR/bus if > $DBUS_SESSION_BUS_ADDRESS is not set. I'd like to limit the amount of hard-coded "magic" that cannot be disabled, configured, overridden, or for that matter, forced to be enabled. With xdg-runtime: as a transport, a system integrator can set the fallback address to be xdg-runtime:, autolaunch:, any combination of the two, or any other hard-coded path of their choice (for instance, an Android app might embed a libdbus configured for unix:path=/data/data/com.example.myapp/session-bus). We offer --with-dbus-session-bus-connect-address because people have needed it in the past, but that mechanism is ineffective if there's a hard-coded fallback to something that can only be changed via patching. You've said yourself that you don't want to support "autolaunch:"; if autolaunch had worked the way that you propose XDG_RUNTIME_DIR should work, then it would not be possible for system integrators to configure libdbus to *not* try autolaunching if DISPLAY was set. In the glorious hypothetical future when we're all using Wayland UIs, with DISPLAY pointing to a compatibility X server for old apps' benefit, I'd like Wayland-based distributions to be able to configure --with-dbus-session-bus-connect-address=xdg-runtime: and lose autolaunch completely (even if dbus upstream still supports it). (In reply to comment #25) > I know this is only test code, but see > https://bugzilla.gnome.org/show_bug.cgi?id=659326 > > To do it safely you could use g_test_trap_subprocess(). I'm pretty sure that test is single-threaded (it doesn't use GDBus), and I'd rather not bump the GLib dependency to 2.38 just yet. (In reply to comment #26) > At a high level I'm uncertain whether or not it makes sense to land patches > like this until the "full picture" of user@.service is clear. Let's defer this to 1.9 then. > That aside, coming back to this now, I find Simon's arguments for explicit > xdg-runtime: make sense. (We could *also* do this automatically if the > environment variable is not set of course) When the environment variable is not set, we behave as if it was set to a compile-time-configurable value, currently defaulting to "autolaunch:". Attachment #75352 [details] adds xdg-runtime: to that default value. Created attachment 91550 [details] [review] Add xdg-runtime: transport This is basically the same as substituting the environment variable into "unix:path=$XDG_RUNTIME_DIR/dbus_user_bus_socket", but doesn't require escaping the XDG_RUNTIME_DIR if it contains special characters such as ':', and the XDG_RUNTIME_DIR can't get out of sync with the DBUS_SESSION_BUS_ADDRESS this way. Created attachment 91551 [details] [review] On Unix, try xdg-runtime: before autolaunch: by default This means that if DBUS_SESSION_BUS_ADDRESS is not set, but XDG_RUNTIME_DIR is, and $XDG_RUNTIME_DIR/bus exists and can be connected to, we will use it instead of X11 autolaunch. (In reply to comment #35) > Add xdg-runtime: transport > > This is basically the same as substituting the environment variable > into "unix:path=$XDG_RUNTIME_DIR/dbus_user_bus_socket" Er, imagine that said ".../bus". (In reply to comment #32) > I'd like to limit the amount of hard-coded "magic" that cannot be disabled, > configured, overridden, or for that matter, forced to be enabled. [...] > We offer --with-dbus-session-bus-connect-address because people have needed > it in the past, but that mechanism is ineffective if there's a hard-coded > fallback to something that can only be changed via patching. Are people convinced by this reasoning? We seem to have me in favour, Colin vaguely in favour and Lennart against. Do any other maintainers have an opinion? Still quite sure I don't like this as transport... Adding a new transport means every dbus implementation have to add support for this new protocol before it can be used. Seems unnecessary harsh for such a small gain vs using "unix:" with an expanded path. However, for *listening* sockets it would be nice to be able to use some special syntax, as you can't really hardcode a path in session.conf that depends on a runtime environment variable. I posted a patch here: http://lists.freedesktop.org/archives/dbus/2014-December/016473.html Which just adds a new listen feature to unix:tmpdir=foo listen addresses that uses XDG_RUNTIME_DIR instead of the specified tmpdir if the env var is set. Otherwise it uses the tmpdir. Something like that, but that uses the fixed socked name instead of the random stuff when in xdg-runtime seems better to me. Then this will be expanded to a regular unix: protocol that all existing implementations already can read. Whether to use the fixed socket address in xdg-runtime if no session address is specified is a different question. I don't have a strong opinion on this. (In reply to Alexander Larsson from comment #40) > Adding a new transport means every dbus implementation have to add support > for this new protocol before it can be used. Seems unnecessary harsh for > such a small gain vs using "unix:" with an expanded path. I get your point, but the idea was that this could be (part of) a new default, and changing defaults (or adding logic for a dynamically constructed default as Lennart proposes) requires library changes anyway. The main issue with unix:path=.../bus (or whatever) as a connectable address, other than needing more information from the environment, is that you can't just blindly substitute XDG_RUNTIME_DIR into a DBUS_SESSION_BUS_ADDRESS, because some characters need escaping. > However, for *listening* sockets it would be nice to be able to use some > special syntax, as you can't really hardcode a path in session.conf that > depends on a runtime environment variable. Note that anything that runs a dbus-daemon (dbus-run-session; user/dbus.service; dbus-launch; gnome-session?) can override the configured listening address via --address if desired. Implementations of user/dbus.service have typically done this; dbus-run-session, dbus-launch do not currently have an --address argument that is passed through to dbus-daemon, but I'd review patches to add one if desired. > I posted a patch here: > http://lists.freedesktop.org/archives/dbus/2014-December/016473.html > > Which just adds a new listen feature to unix:tmpdir=foo listen addresses > that uses XDG_RUNTIME_DIR instead of the specified tmpdir if the env var is > set. I don't like the idea of having unix:tmpdir=/tmp,[more keys] not actually be anything to do with /tmp. Something like unix:runtime=1 would make more sense. The reason we use abstract sockets for the tmpdir "sub-transport" is to avoid building up an unbounded number of stale sockets in /tmp. If you want filesystem-based sockets for more obvious interaction with containers, which it seems you do, then you need either a solution for reaping stale ones (dbus has dbus-cleanup-sockets(1) for non-Linux Unix, but that isn't really a solution), or a way to avoid having an unbounded number of them. Using a fixed relative path in XDG_RUNTIME_DIR seems like a good way to avoid the unbounded number of sockets (by bounding it at a maximum of 1). The ability to support systems with XDG_RUNTIME_DIR unset sounds like a job for <listen_if_possible/> (Bug #50418, which just needs some fixes for broken OOM handling), or leaving unix:tmpdir=/tmp in the configuration and overriding it using the --address option on systems that are known to set XDG_RUNTIME_DIR (e.g. systemd user/dbus.service). Alternatively, giving unix:runtime=1 the semantics of "if XDG_RUNTIME_DIR is set, make a non-abstract socket there, else fall back to behaving as if unix:tmpdir=/tmp" might make some sense? I don't quite see the problem with needing to escape $XDG_RUNTIME_DIR. This would have to be done in two places. First if DBUS_SESSION_BUS_ADDRESS is unset, but this would be done in libdbus (or other dbus implementations). And this is only needed if we want to support the new "default", not for the existing setup with the env var set. Secondly, when creating a listening socket when starting a session dbus daemon. However, this later case could be done with a special listen address type (like unix:runtime=1) which would be expanded by dbus itself to a regular unix:path= address for the clients. Also, for cleanup, even if we don't use a single socket name in XDG_RUNTIME_DIR we get cleanup from the semantics of the dir. From the spec: The lifetime of the directory MUST be bound to the user being logged in. It MUST be created when the user first logs in and if the user fully logs out the directory MUST be removed. Anyway, I'm not super excited about the new default. Things work with the existing envvar already. Seems like a pretty minor cleanup. Not using abstract sockets however is pretty important for containerized apps, as such apps will be completely unable to access the session bus if they have a private network, etc. (In reply to Simon McVittie from comment #41) > I don't like the idea of having unix:tmpdir=/tmp,[more keys] not actually be > anything to do with /tmp. Something like unix:runtime=1 would make more > sense. I've implemented this. > The ability to support systems with XDG_RUNTIME_DIR unset sounds like a job > for <listen_if_possible/> This is not actually necessary, assuming you only want to listen in one place: you can already say <listen>unix:runtime=1;unix:tmpdir=/tmp</listen> and you'll get unix:path=/run/whatever/bus if XDG_RUNTIME_DIR is /run/whatever, or unix:abstract=/tmp/something otherwise. I don't think we need (or want) to support having more than one concurrent D-Bus session sharing an XDG_RUNTIME_DIR, so it's OK to have the relative path be something fixed, like "./bus"; that makes life simpler. (Yes, Lennart, before you say it, I know you want kdbus instead, but people are already using dbus-daemon as a user bus, and it seems like it makes more sense if they can all be using upstreamed code.) Created attachment 111959 [details] [review] Add support for unix:runtime=yes as an address mode This listens on $XDG_RUNTIME_DIR/bus, or fails if $XDG_RUNTIME_DIR is not set. Fallback behaviour is unnecessary, because it is already possible to use a string of semicolon-separated addresses like <listen>unix:runtime=yes;unix:tmpdir=/tmp</listen>, resulting in listening on either $XDG_RUNTIME_DIR/bus or /tmp/something. We use a non-abstract socket here, because that is desirable for use with Linux containers: abstract sockets are attached to the network namespace, whereas non-abstract sockets are part of the filesystem and can be bind-mounted between domains if necessary. The major advantage of abstract sockets is that they do not need cleanup, but the specification of XDG_RUNTIME_DIR guarantees to provide cleanup anyway. Based on prior work by Simon McVittie, Colin Walters and Alexander Larsson. --- I think this obsoletes all the parts of Attachment #89097 [details] that are not Bug #61301, except for the change to _dbus_lookup_session_address(), which still needs altering to escape the address. It also obsoletes the server-side part of Attachment #91550 [details]. Created attachment 111960 [details] [review] Add regression test for unix:runtime=1 (In reply to Alexander Larsson from comment #40) > Adding a new transport means every dbus implementation have to add support > for this new protocol before it can be used. Actually, it doesn't: in exactly the same way that if you run "dbus-daemon --address=unix:tmpdir=/tmp --print-address=1" it prints the connectable address "unix:abstract=/tmp/wtftbh" instead of the listenable address "unix:tmpdir=/tmp", with my earlier patches, I'm fairly sure "dbus-daemon --address=xdg-runtime: --print-address=1" would have printed something like "unix:path=/run/user/1000/bus". The same is true for my more recent server-side-only unix:runtime=yes patch, which is basically a different way to spell xdg-runtime:. The only component that needs to know about the new transport or option is the component that will run "dbus-daemon --address=unix:runtime=yes" or "dbus-daemon --address=xdg-runtime:", which in practice is usually going to be a systemd unit; it has traditionally been some third-party thing like user-session-units, but that's a bug (specifically Bug #61301) and I really think it should be dbus' job. The major difference between the server-side-only and server-and-client-side versions is that if a component *does* know about the new transport or option, it can use xdg-runtime: (or maybe unix:runtime=yes) as a shorthand for "connect to the correct bus socket". Created attachment 112940 [details] [review] Add regression test for unix:runtime=1 --- Attachment #111960 [details] was an older version which expected runtime=1 instead of runtime=yes, so the test failed. Created attachment 112942 [details] [review] On Unix platforms, try $XDG_RUNTIME_DIR/bus before default address Based on part of a patch by Colin Walters. Changes: - set error correctly on OOM - escape the path if it contains inconvenient characters - coding style adjustments --- This is essentially the parts of Attachment #89097 [details] that are not already on Bug #61301, with some fixes. I still think either xdg-runtime: (Attachment #91550 [details], Attachment #91551 [details]) or unix:runtime=yes is a better spelling for this feature than an implicit default that only works for dbus_bus_get*(), cannot be configured out, and cannot be connected to any other way; I'd have preferred it if launchd worked like this too. However, if Lennart wants to veto the "it's a transport just like autolaunch:" approach to the client-side, I'm willing to compromise. To make this usable for existing distributions without breaking compatibility with /etc/Xsession.d or equivalent, I think we probably also need something like the dbus-update-activation-environment tool that I alluded to on the systemd/dbus mailing lists. I'm going to work on that on the train on my way to the systemd hackfest. Created attachment 112943 [details] [review] Add a regression test for connecting to XDG_RUNTIME_DIR/bus by default Comment on attachment 112942 [details] [review] On Unix platforms, try $XDG_RUNTIME_DIR/bus before default address This patch is not right. Taking this bit to Bug #61301 instead. Created attachment 113366 [details] [review] [1/2] Add support for unix:runtime=yes as an address mode This is not used by default, but can be configured by OS builders (or regression-test environments) if desired. If used, this listens on $XDG_RUNTIME_DIR/bus, or fails if $XDG_RUNTIME_DIR is not set. Fallback behaviour is unnecessary, because it is already possible to use a string of semicolon-separated addresses like <listen>unix:runtime=yes;unix:tmpdir=/tmp</listen>, resulting in listening on either $XDG_RUNTIME_DIR/bus or /tmp/something. We use a non-abstract socket here, because that is desirable for use with Linux containers: abstract sockets are attached to the network namespace, whereas non-abstract sockets are part of the filesystem and can be bind-mounted between domains if necessary. The major advantage of abstract sockets is that they do not need cleanup, but the specification of XDG_RUNTIME_DIR guarantees to provide cleanup anyway. Based on prior work by Simon McVittie, Colin Walters and Alexander Larsson. Created attachment 113367 [details] [review] [2/2] Add regression test for unix:runtime=yes Comment on attachment 113367 [details] [review] [2/2] Add regression test for unix:runtime=yes Review of attachment 113367 [details] [review]: ----------------------------------------------------------------- Spotted a minor leak in OOM situations while reviewing my own code: ::: dbus/dbus-server-unix.c @@ +112,5 @@ > + _dbus_string_init_const (&filename, "bus"); > + > + if (!_dbus_string_init (&full_path) || > + !_dbus_string_append (&full_path, runtimedir) || > + !_dbus_concat_dir_and_file (&full_path, &filename)) This needs to be split into two blocks. If the _dbus_string_init() succeeds, but then one of the others fails, then we need to free full_path before raising the OOM error; and we can't free full_path unconditionally, because DBusString does not have a safe static initializer that can be "freed" without error. @@ +126,5 @@ > + _dbus_string_get_const_data (&full_path), > + FALSE, error); > + > + _dbus_string_free (&full_path); > + _dbus_string_free (&filename); _dbus_string_free (&filename); is unnecessary (although harmless), because an init_const'd DBusString does not allocate any dynamic memory. Comment on attachment 113366 [details] [review] [1/2] Add support for unix:runtime=yes as an address mode Review of attachment 113366 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-server-unix.c @@ +114,5 @@ > + if (!_dbus_string_init (&full_path) || > + !_dbus_string_append (&full_path, runtimedir) || > + !_dbus_concat_dir_and_file (&full_path, &filename)) > + { > + _DBUS_SET_OOM (error); Do you not want _dbus_string_free() in this block, in case the failure was in _dbus_string_append() or _dbus_concat_dir_and_file()? ::: doc/dbus-specification.xml @@ +3178,5 @@ > + <row> > + <entry>runtime</entry> > + <entry><literal>yes</literal></entry> > + <entry>If given, This key can only be used in server addresses, not in client addresses. If set, its value must be <literal>yes</literal>. This is typically used in an address string like <literal>unix:runtime=yes;unix:tmpdir=/tmp</literal> so that there can be a fallback if <literal>XDG_RUNTIME_DIR</literal> is not set.</entry> > + </row> Need to add ‘runtime’ to paragraph 4 of §(Unix Domain Sockets) above. Might also want to mention it in §(Server Addresses) to clarify that it is listenable and(?) connectable. Comment on attachment 113367 [details] [review] [2/2] Add regression test for unix:runtime=yes Review of attachment 113367 [details] [review]: ----------------------------------------------------------------- Wrong patch? This doesn’t contain a regression test! (In reply to Philip Withnall from comment #54) > Do you not want _dbus_string_free() in this block, in case the failure was > in _dbus_string_append() or _dbus_concat_dir_and_file()? Yes, I spotted the same thing. > Need to add ‘runtime’ to paragraph 4 of §(Unix Domain Sockets) above. Indeed. > Might also want to mention it in §(Server Addresses) to clarify that it is > listenable and(?) connectable. It is in fact listenable but not currently connectable, and makes another good example. Documented as such. Created attachment 113401 [details] [review] Add support for unix:runtime=yes as an address mode --- See http://cgit.freedesktop.org/~smcv/dbus/log/?h=fixup/unix-runtime-61303 for individual fixes. Created attachment 113402 [details] [review] Add regression test for unix:runtime=yes Created attachment 113415 [details] [review] loopback test: cope with being run without XDG_RUNTIME_DIR This could happen during installed-tests. --- From installed-system testing. Can be squashed into Attachment #113402 [details] if desired. Comment on attachment 113415 [details] [review] loopback test: cope with being run without XDG_RUNTIME_DIR Er, this would work better if the check was the other way round. Created attachment 113417 [details] [review] loopback test: cope with being run without XDG_RUNTIME_DIR This could happen during installed-tests. --- Could be squashed into Attachment #113402 [details] if desired. Created attachment 113759 [details] [review] [2/2] Add regression test for unix:runtime=yes --- This is just Attachment #113402 [details] and Attachment #113417 [details] squashed together for easier review. Nobody has actually objected to unix:runtime=yes, and the test Bug #61301 needs it, so I'm going to apply these patches soon if nobody says no. Comment on attachment 113401 [details] [review] Add support for unix:runtime=yes as an address mode Review of attachment 113401 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 113759 [details] [review] [2/2] Add regression test for unix:runtime=yes Review of attachment 113759 [details] [review]: ----------------------------------------------------------------- ++ Fixed in git for 1.9.14. The new transport mode can be used to listen but not to connect, and is spelled "unix:runtime=yes". The resulting "connectable" address is "unix:path=/run/user/1000/bus" or whatever. |
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.