[patch] Auto session bus launch from libdbus

[Havoc Pennington]

Thiago Macieira wrote:
> 
> * I have no idea if flock(2) is portable to the other Unix platforms or if 
> it's enough to guarantee exclusive access on NFS mounts. Someone with 
> better knowledge, please come forward.
>

Sadly I have way too much knowledge of this ;-) Struggled with it for a 
long time for gconfd, which does the auto-launch thing. This is maybe 
the biggest reason I suggested the X property instead of the file.

flock() does not even try to work over NFS. The normal solution is to 
use fcntl() instead, which is supposed to work on NFS.

fcntl() theoretically works, but in real life a bunch of sysadmins have 
it hosed for various reasons; they firewalled the lock daemon, their 
distribution got the lock daemon default configuration wrong, bugs in 
the lock daemon or NFS implementation, or their NFS server is some crap 
thing that doesn't even have a lock daemon (older unixes, for example).

Anyway... basically, this is going to work in testing, but when deployed 
to the real world there will be a number of bug reports complaining 
about it.

Since autostart with dbus is just a kind of busted fallback if your 
distribution didn't set up X properly, this could be OK I guess, as long 
as we proceed somehow if the locking fails.

You might use the approach from dbus-keyring.c:_dbus_keyring_lock(), 
except that this approach assumes a transitory lock - i.e. it's timeout 
based, but would normally hold the lock for only milliseconds. A lock 
that would be held for a long time can't be timeout-based like this.

Some thoughts on the patch -

> Index: dbus/dbus-bus.c
>  
> +#include "config.h"
> +
>  #include "dbus-bus.h"
>  #include "dbus-protocol.h"
>  #include "dbus-internals.h"

config.h should be in dbus-internals.h anyhow

> +#ifdef DBUS_BUILD_X11
> +/* This function is mirrored in dbus-launch.c
> +   Don't modify it here without modifying it there */

Ideally in some way we can avoid the cut-and-paste in a final patch, 
though the implementations aren't really the same :-/ Maybe dbus-launch 
should just be linking in the convenience lib same as some of the other 
tools.

> +static DBusString
> +get_session_file()

Should be:
static dbus_bool_t
get_session_file(DBusString *filename)

remember () has to be (void) in C

> +  display = _dbus_strdup (_dbus_getenv ("DISPLAY"));

There is a problem here, in that DISPLAY does not have to be the 
canonical display name. Here is an old post on that:
http://mail.gnome.org/archives/nautilus-list/2002-March/msg00185.html

DISPLAY will just be :0.0 much of the time. You're adding the hostname 
unconditionally, really it should be added only if it isn't already in 
the display, e.g. if using a remote display you'd expect to have 
DISPLAY=hostname:0.0 already in the env variable.

To get the hostname-containing version with Xlib, you can do 
DisplayString(display), but we don't have Xlib sadly.

Even if we add the hostname (iff it isn't there already), we have the 
problem that the hostname is often localhost.localdomain.

So collisions are not unlikely here I would say...

An approach (other than the X property that avoids some of this mess 
;-)) might be to have dbus create a per-machine guid at install time. 
This is just a useful thing to have available anyway, I've been thinking 
of adding it to the system bus. Then if we parsed the ":N" part out of 
DISPLAY and appended it to the machine guid I believe it is pretty robust.

This is trivial to do, we would just provide dbus-uuidgen and say it has 
to be in the postinstall of dbus packages. (dbus-uuidgen is the same as 
the "uuidgen" command but we can't rely on that existing.)

> +  _dbus_string_alloc_space (&result, strlen (home) + strlen (prefix) + 
> +			    strlen (_dbus_string_get_const_data (&hostname)) +
> +			    strlen (display) + 2);

This isn't needed - the below append() calls will do it automatically. 
(The point of DBusString is basically to avoid string.h and manual 
calculations that might be wrong, an idea taken from vsftpd)

> +  _dbus_string_append (&result, home);
> +  _dbus_string_append (&result, prefix);
> +  _dbus_string_append (&result, _dbus_string_get_const_data (&hostname));
> +  _dbus_string_append_byte (&result, '_');
> +  _dbus_string_append (&result, display);

All of these will need a check for OOM if the alloc_space is removed though.

> +static dbus_bool_t
> +check_address (const char *address, char **connection_p)

This seems a bit odd, creating transports without connections... why not 
just try dbus_connection_open then keep the resulting connection if it 
works?

> +  char *argv[] = { dbus_launch_full, "--exit-with-session", NULL };

I think a --launch-for-session might be nice in dbus-launch; even if 
it's the same as --exit-with-session now, I can imagine reasons that it 
won't always be. For example, the below code could be cleaned up by 
using a pipe to signal the launched bus is ready, instead of spinning in 
a loop looking for the session file. So if there's a 
--launch-for-session it would know about the pipe thing for example.

This function that spawns dbus-launch is really very unix-specific in 
practice, so we'll have to figure that out. Well, really the whole 
mechanism here is very UNIX-specific. Maybe what we want is something like:

  char* _dbus_find_bus_address(DBusBusType bus_type);

in dbus-sysdeps.h, where it looks in additional platform-specific 
locations for the bus address, possibly launching a new bus.

> +dbus_bool_t
> +_dbus_spawn_async (const char **argv, dbus_bool_t use_path, DBusError *error)

Seems likely to be possible to share code here with existing dbus-spawn.c?

Oddly enough dbus-spawn.c is a cut-down version of glib/gspawn.c which I 
also wrote and which had the function you need here, so I deleted this 
from dbus-spawn.c at some point ;-)

> +++ tools/dbus-launch.c	10 Sep 2006 21:24:08 -0000
> @@ -32,8 +32,11 @@
>  #include <signal.h>
>  #include <stdarg.h>
>  #include <sys/select.h>
> +#include <time.h>
>  #ifdef DBUS_BUILD_X11
>  #include <X11/Xlib.h>

hmm, with this thing already including Xlib.h, isn't it just as easy to 
use an X property? I guess the downside is that a fork is always 
required in libdbus, instead of only when the bus needs starting.

Thoughts on that though:

  - we could still have the file, but don't use it for locking - just
    use it as an optimization, so the algorithm could be:
    - if DBUS_SESSION_BUS_ADDRESS, try to connect
    - if that fails and session file, try to connect to address in file
    - if that fails, try running dbus-launch and reading the address
      back from it via a pipe
    In this algorithm, stale files just don't matter; anytime
    the bus is started, we use the X server for locking.

  - the normal case by far is that DBUS_SESSION_BUS_ADDRESS is set
    anyway, to me autolaunch is used when the distribution vendor
    was too lazy to launch the bus from their X startup scripts.
    e.g. Fedora launches the bus for any kind of session (gnome,kde,twm).
    So basically it's a bug if autolaunch is used, making it work is
    nice I suppose, but I feel no compulsion to save 50 milliseconds by
    avoiding a fork() ;-)

  - the other uses cases I can think of are:
    - ssh to a remote system
    - su to another user
    Both of those have all kinds of possible breakage, with either
    autolaunch or DBUS_SESSION_BUS_ADDRESS forwarding, but ignoring
    that - again I don't think 50 milliseconds will hurt anything.

> +#include <sys/file.h>
> +#include <pwd.h>

(shows how unix-specific dbus-launch is, I think we should probably 
implement this whole thing in dbus-sysdeps-unix.c more or less as I 
mentioned earlier)

>  static void
> -kill_bus_when_session_ends (void)
> +kill_bus_when_session_ends ()

(C++ ism)

Havoc