[patch] Auto session bus launch from libdbus, take 2
Thiago Macieira
thiago at kde.org
Sat Sep 16 02:25:02 PDT 2006
Havoc Pennington wrote:
>> 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?
Yes, I plan to. But since this is just an optimisation, I thought I would
finish the basic parts first and then implement that.
>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 ;-)
Hmm... the system bus side fails completely if the strdup failed. I guess
I could do the same here.
>> +#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 had first intended to call it "system" (as in system-dependent), but
then it would be confusing that the default session bus address
is "system". So I chose "session". But the name does not look good.
Since this is an internal token, I even considered changing it
to "the_magic_method_that_you_should_never_see". :-)
>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.
Or not set the variable at all. If we don't export this method, you can
only get autostarting by unsetting the variable.
>> +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.
Indeed. _dbus_transport_open should pass control to the platform-specific
version if the none of the known protocols match. That will probably be
just TCP or not even that. AFAIR from Winsocks, it's so different and so
full of pitfalls that it should probably be in a separate file anyways.
That would mean almost all of DBusTransport would be platform-specific.
(cf the email someone has just posted on the subject)
>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 don't think dbus-transport-socket.c would contain much code at all.
>> +{
>> + 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.
Not really. dbus-bus.c still knows only "session:" and there are private
session busses. But since this cache is a potential memory leak because
we don't have an atomic test-and-set operation available, maybe it's
better not to have it at all. Once the session file is read, the fork
penalty should go away.
>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 ?
Agreed.
>> + /* 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.
Ok.
>> + 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)
Right, it doesn't. I thought it did.
>> + 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.
I mixed a bit of the autolaunch idea with the system-specific protocols.
I'll go back to system-specific and that should be enough.
>> #ifdef DBUS_BUILD_X11
>> #include <X11/Xlib.h>
>> +extern Display *xdisplay;
>> #endif
>
>just put this in dbus-launch.h ?
From past knowledge: X11/Xlib.h is an evil header, so always include it
last.
>> + 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.
If you say so. I'm used to having the "WId" typedef as part of the API in
Qt.
>> + 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 ?
--exit-wth-session already implies "exit with X session".
Maybe
--enable-x11-integration?
--enable-autolaunch?
--autolaunch?
If we don't say anything about X11 there, the code on libdbus would
require no modifications to support MacOS X mechanisms.
>> 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
Oops, copy-paste :-)
>> +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.
If you call your host localhost, it's not very likely you're going to use
remote X sessions. And the likelihood of you having a localhost host and
connecting to another localhost and sharing X is very low.
>> + 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.
No, this is correct. The session file is per user, per host and per
display. If I am on machine A.net and my DISPLAY is B.net:0.0, then the
file should be called A.net_B.net_0.
A.net__0 means display :0 running on A.net (local display)
B.net__0 means display :0 running on B.net (local display)
>> + 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
Sure. It's not really a problem :-)
>Why do all three of these need to be set? I would think we only need the
>address?
The selection doesn't hold a value per-se. It's used by XGetSelectionOwner
to identify the window that has the other two properties.
The fact that we're using a selection and a window instead of storing the
properties in the root window means we don't have to worry about stale
data. If dbus-launch crashes, exits or is killed, the window will go
away, along with the selection, automatically.
BTW: dbus-daemon can sometimes exit and dbus-launch doesn't realise that.
I guess it's a bug.
>> + /* 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.
That's already like that. The property atoms don't contain the hostname.
>
>> + 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.
If sizeof(pid_t) == 8, then nitems will be 2. However, I hadn't thought of
unaligned access.
>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 ;-)
>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.
Well, since dbus-launch is the only program using that selection, I guess
we can depend on it being nice to itself. It'll never steal the
selection: after it grabs the server, it verifies once again that it
hasn't started in the mean time. If it has, then it kills the bus and
exits (.... huh, without showing the address to the caller...). Since the
server is grabbed, no one can steal the session between that verification
and the actual selection setting.
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20060916/e0e04965/attachment.pgp
More information about the dbus
mailing list