[patch] Auto session bus launch from libdbus, take 2

[Havoc Pennington]

Hi,

Thiago Macieira wrote:
> Here goes another patch.

Looks good to me, I only have nitpicky/what-things-are-named kind of 
comments, no major structural changes.

> Worthy of note:
> * the session file is created, but never erased
> * the session file is never read from (libdbus-1 always forks and calls 
> dbus-launch)

Is the intent that we would add code to libdbus to read it?

Here are my nitpicks:

> +	  if (bus_connection_addresses[DBUS_BUS_SESSION] == NULL)
> +	    bus_connection_addresses[DBUS_BUS_SESSION] =
> +	      _dbus_strdup (DBUS_SESSION_BUS_DEFAULT_ADDRESS);
> +
>            _dbus_verbose ("  \"%s\"\n", bus_connection_addresses[DBUS_BUS_SESSION] ?
>                           bus_connection_addresses[DBUS_BUS_SESSION] : "none set");

This effectively handles out of memory OK, but it's a little odd since 
the code looks like it's not ever supposed to have a null 
DBUS_BUS_SESSION, even though it does work. I can't think of a fix 
really so I guess I'm just whining ;-)

> +#define DBUS_SESSION_BUS_DEFAULT_METHOD		"session"
> +#define DBUS_SESSION_BUS_DEFAULT_ADDRESS	DBUS_SESSION_BUS_DEFAULT_METHOD ":"

This gives me an idea, which is to go ahead and define methods 
"autolaunch" and "homefile" then the default address could be
"homefile:,autolaunch:" thus getting the automatic fallback behavior.

I think this was a great idea to define autostart as an address method, 
which means DBUS_SESSION_BUS_ADDRESS replaces it completely; if you have 
  a bad address env variable, an error is better than autostarting a new 
bus. But if people want to force autostart they can put the autostart 
address in their env variable.

This new address type(s) should be mentioned in the man page. Well, I'm 
not sure the old addresses are in there, but I hope they are. Surely 
they are somewhere in the docs. Wherever they are in the docs, we should 
also mention any new address types there, let's put it that way ;-)

> +DBusTransport *
> +_dbus_transport_open_sysdep (DBusAddressEntry *entry,
> +			     DBusError *error)


The unix domain socket transport is also unix-specific, but
isn't (for the moment) in this file. presumably we want to
handle these two or three platform-specific methods
(unix:,session:/homefile:/autolaunch:) in a similar way.
I'm not sure what that way is yet but the rough theory
I had was to move most of dbus-transport-unix.c to a 
dbus-transport-socket.c also used on Windows and have it handle tcp:. 
Then have dbus-transport-unix.c just handle unix: and chain to 
dbus-transport-socket.c.

I would not want to mix the dbus-transport-socket.c change into this 
patch, but the following might make sense:
  - export:
     char* _dbus_autolaunch_bus_unix(DBusError *error)
    which returns the address of the autolaunched bus.
  - use that function in dbus-transport-unix.c to implement
    _dbus_transport_new_autolaunch()

Then the code will stay about the same when we clean it up to have 
dbus-transport-socket.c.

> +{
> +  static char *cached_address = 0;

I'm not sure this cache is needed, since the session bus is stored 
globally in dbus-bus.c, once the connection is open we won't try to open 
the transport again. The cache would however prevent the bus from ever 
being respawned if killed, and it will also break dbus_shutdown(). So 
maybe it's best to just skip it until it shows up in a profile ?

> +	  /* child process */
> +	  int fd = open ("/dev/null", O_RDWR);
> +
> +	  /* set-up stdXXX */
> +	  close (address_pipe[READ_END]);
> +	  close (0);		/* close stdin */
> +	  close (1);		/* close stdout */
> +	  close (2);		/* close stderr */
> +	  dup2 (fd, 0);
> +	  dup2 (address_pipe[WRITE_END], 1);
> +	  dup2 (fd, 2);
> +	  close (fd);
> +	  close (address_pipe[WRITE_END]);

It makes me a tiny bit nervous to ignore all those return values, while 
I agree they won't fail in practice, not printing any errors could hide 
bugs or something. Not a mandatory fix but would be nice.

> +      do 
> +	{
> +	  ret = _dbus_read (address_pipe[READ_END], &address, 1024);
> +	}
> +      while (ret == 1024);

Technically read() is allowed to return <1024 even if EOF has not been 
reached, isn't it? Unless _dbus_read calls read() multiple times until 
it fills the buffer (don't remember)

> +  else if (strcmp (method, DBUS_SESSION_BUS_DEFAULT_METHOD) == 0)
> +    {
> +      transport = _dbus_transport_open_sysdep (entry, error);
> +    }

This could just be _new_autolaunch or whatever here

I think using the DEFAULT_METHOD macro is not right here because here we 
want this specific method, not the default method. i.e. maybe the 
default method could be changed to foobar: but still we want to handle 
session: or autolaunch: in this spot.

>  #ifdef DBUS_BUILD_X11
>  #include <X11/Xlib.h>
> +extern Display *xdisplay;
>  #endif

just put this in dbus-launch.h ?

> +      if (bus_wid)
> +	printf ("DBUS_SESSION_BUS_WID=%ld;\n", (long) bus_wid);
> +    }

I'd spell out instead of the WID abbreviation, PID is a standard 
abbreviation but WID not so much.

> +      else if (strcmp (arg, "--x11-integration") == 0)
> +	x11_integration = TRUE;

I think something more descriptive than --x11-integration would be good.
Maybe --exit-with-x-session and/or --use-existing-x-property ?

Should be sure the option is added to the man page, whatever it is called.

>        verbose ("dbus-launch exiting\n");
> --- /dev/null	2006-09-12 17:30:44.756285750 +0200
> +++ dbus-launch.h	2006-09-15 14:34:19.000000000 +0200
> @@ -0,0 +1,56 @@
> +/* -*- mode: C; c-file-style: "gnu" -*- */
> +/* dbus-launch.c  dbus-launch utility

This should be dbus-launch.h

> --- /dev/null	2006-09-12 17:30:44.756285750 +0200
> +++ dbus-launch-x11.c	2006-09-15 15:56:18.000000000 +0200
> @@ -0,0 +1,387 @@
> +/* -*- mode: C; c-file-style: "gnu" -*- */
> +/* dbus-launch.c  dbus-launch utility

dbus-launch-x11.c

> +static char *
> +get_local_hostname (void)

I do still think the guid approach would be easy and more robust, given 
all the localhost.localdomain hosts in the world. However, I guess 
there's no real compatibility guarantee on how we do this so we can 
change it later.

> +  display = xstrdup (getenv ("DISPLAY"));
> +  if (display == NULL)
> +    {
> +      verbose ("X11 integration disabled because X11 is not running\n");
> +      return NULL;
> +    }
> +
> +  /* remove the screen part of the display name */
> +  p = strrchr (display, ':');
> +  if (p != NULL)
> +    for ( ; *p; ++p)
> +      if (*p == '.')
> +        {
> +          *p = '\0';
> +          break;
> +        }
> +
> +  /* replace the : in the display with _ */
> +  for (p = display; *p; ++p)
> +    if (*p == ':')
> +      *p = '_';
> +
> +  hostname = get_local_hostname ();

Remember the display can also have the hostname in it already, really 
what you want is ONLY the display number which is between the ":" and 
the "." I think.

> +  static const char selection_prefix[] = "D-BUS_SESSION_SELECTION_";
> +  static const char address_prefix[] = "D-BUS_SESSION_ADDRESS";
> +  static const char pid_prefix[] = "D-BUS_SESSION_PID";

Hyphens are rarely used in X atoms, also a "_" prefix is normal, so 
something like _DBUS_SESSION_SELECTION, _DBUS_SESSION_PID

Why do all three of these need to be set? I would think we only need the 
address?

> +  /* create the address property atom */
> +  strcpy (atom_name, address_prefix);
> +  address_atom = XInternAtom (display, atom_name, FALSE);
> +
> +  /* create the PID property atom */
> +  strcpy (atom_name, pid_prefix);
> +  pid_atom = XInternAtom (display, atom_name, FALSE);

Rather than creating all these atoms with the hostname munged in, since 
we are already creating a window to own the selection, we could just 
have the hostname in the selection name, then set the pid and address on 
the selection owner window. That's a pretty normal X thing to do.

> +      XGetWindowProperty (xdisplay, owner, pid_atom, 0, sizeof pid, False,
> +                          XA_CARDINAL, &type, &format, &items, &after,
> +                          (unsigned char **) &data);
> +      if (type != None && after == 0 && data != NULL && format == 32)
> +        *pid = *(pid_t*)data;

I think XGetWindowProperty has surprising behavior on 64-bit but I don't 
remember offhand what it is. It's possible that "data" there is always a 
long even though it's format "32" - I'd have to look at some other code 
and see.

I would not in any case cast to pid_t since that pointer is going to be 
to maybe int or long, but not to pid_t. The cast to pid_t would need to 
be *after* dereferencing the pointer if the compiler is going to do the 
right thing. (Assuming pid_t is anything other than int ever, which I 
doubt, but if going to the trouble to use all the libc _t types, may as 
well do it right ;-)

> +  /* Now grab the selection */
> +  XSetSelectionOwner (xdisplay, selection_atom, wid, CurrentTime);

CurrentTime is one big race condition usually but should be OK since the 
server is grabbed.

Normally when using selections for something like this, one also handles 
losing the selection. So e.g. if we lost the selection we'd kill the 
bus. This is how "--replace" works in window managers; twm and metacity 
both have example code I know.

Probably not worth the time to code it.

Havoc