Bug 14259 - add launchd auto-launch support
Summary: add launchd auto-launch support
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All Mac OS X (All)
: highest critical
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords:
: 16791 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-01-26 11:49 UTC by Benjamin Reed
Modified: 2010-12-08 15:22 UTC (History)
12 users (show)

See Also:
i915 platform:
i915 features:


Attachments
add basic support for launchd socket provisioning (14.95 KB, patch)
2008-01-26 11:51 UTC, Benjamin Reed
Details | Splinter Review
create a launchagent file for starting dbus on Mac OS X (3.87 KB, patch)
2008-01-26 11:51 UTC, Benjamin Reed
Details | Splinter Review
shell syntax change (1.20 KB, patch)
2008-01-26 11:52 UTC, Benjamin Reed
Details | Splinter Review
add OnDemand=false to launchd file (870 bytes, patch)
2008-01-27 08:36 UTC, Benjamin Reed
Details | Splinter Review
full patch with launchd support (8.78 KB, patch)
2008-01-28 10:14 UTC, Benjamin Reed
Details | Splinter Review
whups -- proper full patch (19.77 KB, patch)
2008-02-12 13:41 UTC, Benjamin Reed
Details | Splinter Review
relicense as MIT (5.28 KB, patch)
2008-02-12 13:46 UTC, Benjamin Reed
Details | Splinter Review
Updated patch addressing comments (18.45 KB, patch)
2008-05-15 20:26 UTC, Tanner Lovelace
Details | Splinter Review
lots of refactoring (32.52 KB, patch)
2008-06-11 13:23 UTC, Colin Walters
Details | Splinter Review
also remove obsoleted function (3.02 KB, patch)
2008-06-11 13:41 UTC, Colin Walters
Details | Splinter Review
add autolaunch test case (3.13 KB, patch)
2008-06-11 15:04 UTC, Colin Walters
Details | Splinter Review
Fix argument ordering (6.12 KB, patch)
2008-06-11 15:05 UTC, Colin Walters
Details | Splinter Review
cleanup and fixes, removed "default:" address type (30.31 KB, patch)
2009-01-09 18:47 UTC, Jonas Bähr
Details | Splinter Review
new full patch, addressing havoc's comments (31.87 KB, patch)
2009-01-10 17:06 UTC, Jonas Bähr
Details | Splinter Review
Next version of the patch, now with README and launching services fixed (40.13 KB, patch)
2009-01-19 18:18 UTC, Jonas Bähr
Details | Splinter Review
new version of the full patch (25.29 KB, patch)
2009-02-23 05:38 UTC, Benjamin Reed
Details | Splinter Review
final version of the patch (40.86 KB, patch)
2009-05-30 09:59 UTC, Benjamin Reed
Details | Splinter Review
full patch with Fink fix (40.86 KB, patch)
2009-07-07 12:45 UTC, Benjamin Reed
Details | Splinter Review
make the session bus <listen> tag configurable (1.34 KB, patch)
2009-07-21 15:27 UTC, Benjamin Reed
Details | Splinter Review
the main launchd implementation (11.41 KB, patch)
2009-07-21 15:27 UTC, Benjamin Reed
Details | Splinter Review
look up DISPLAY from launchd if it is not initialized (1.90 KB, patch)
2009-07-21 15:28 UTC, Benjamin Reed
Details | Splinter Review
enable launchd in configure, and tie it into dbus-server-unix (14.19 KB, patch)
2009-07-21 15:29 UTC, Benjamin Reed
Details | Splinter Review
enable launchd in configure, and tie it into dbus-server-unix (14.51 KB, patch)
2009-07-22 05:02 UTC, Benjamin Reed
Details | Splinter Review
the main launchd implementation (11.90 KB, patch)
2009-07-22 05:22 UTC, Benjamin Reed
Details | Splinter Review
Tanner's patch for using a DBusString for the socket (6.39 KB, patch)
2009-07-22 15:48 UTC, Benjamin Reed
Details | Splinter Review
make the session bus <listen> tag configurable (1.34 KB, patch)
2009-07-29 08:11 UTC, Benjamin Reed
Details | Splinter Review
the main launchd implementation (12.13 KB, patch)
2009-07-29 08:11 UTC, Benjamin Reed
Details | Splinter Review
look up DISPLAY from launchd if it is not initialized (1.95 KB, patch)
2009-07-29 08:11 UTC, Benjamin Reed
Details | Splinter Review
enable launchd in configure, and tie it into dbus-server-unix (14.99 KB, patch)
2009-07-29 08:12 UTC, Benjamin Reed
Details | Splinter Review
add autolaunch test case (3.18 KB, patch)
2009-07-29 08:12 UTC, Benjamin Reed
Details | Splinter Review
Git repositories (137 bytes, text/plain)
2010-12-03 13:11 UTC, Mike McQuaid
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Reed 2008-01-26 11:49:46 UTC
Add support for auto-launching the session bus with launchd on Mac OS X.  In theory, this could apply to launchd ported to other platforms, but I am not set up to test that.
Comment 1 Benjamin Reed 2008-01-26 11:51:03 UTC
Created attachment 13957 [details] [review]
add basic support for launchd socket provisioning

Add support for launchd (http://developer.apple.com/macosx/launchd.html)
Launchd is superficially like inetd in that it controls all resources for
system-wide and session-level daemons.

In the case of this dbus implementation, launchd allocates and listens on
a socket, dynamically launching dbus when a client tries to access the
socket for the first time.  When dbus-daemon is started, it asks launchd
for the socket file descriptor, and then initializes itself using that
pre-existing FD.  Note that this only occurs if <listen> in session.conf
is set to "autolaunch:" -- it is still possible to us DBus on Mac OS X
in the normal UNIXy way by specifying a tcp: or unix: session address.

From the client-side, if DBUS_SESSION_BUS_ADDRESS is set, the client will
connect normally, just like any other unix client.  If it is not, it
checks for the environment variable DBUS_LAUNCHD_SESSION_BUS_SOCKET and
crafts a unix: socket URL from it, and then continues normally.
Comment 2 Benjamin Reed 2008-01-26 11:51:42 UTC
Created attachment 13958 [details] [review]
create a launchagent file for starting dbus on Mac OS X

Add a file which tells launchd how to auto-launch the dbus
session daemon.  Also, add --with-launchd-agent-dir to allow
overriding of the default /Library/LaunchAgents location.
Comment 3 Benjamin Reed 2008-01-26 11:52:38 UTC
Created attachment 13959 [details] [review]
shell syntax change

a minor shell syntax change to make 10.4's /bin/sh happy
Comment 4 John (J5) Palmieri 2008-01-26 12:59:37 UTC
I'm wondering if this can be done in dbus-launch and not in the bus itself.
Comment 5 Benjamin Reed 2008-01-26 13:04:23 UTC
I suppose so; it just seemed a bit silly to have dbus-launch do it when launchd already basically does what dbus-launch does (sets up an environment and determines how to launch the daemon).

This way you can still do things the traditional dbus-launch unixy way and have it all work the same; it only has a different behavior if enabled in launchd.
Comment 6 Benjamin Reed 2008-01-26 13:07:58 UTC
Also note, launchd daemons are not supposed to fork, so you'd be leaving up a shunt binary (dbus-launch) whose only purpose is to stay open and keep dbus-daemon open.  :P
Comment 7 Benjamin Reed 2008-01-27 08:36:52 UTC
Created attachment 13971 [details] [review]
add OnDemand=false to launchd file

on-demand launching is buggy on 10.4, so set to always start with the user's session
Comment 8 Benjamin Reed 2008-01-28 10:14:44 UTC
Created attachment 13990 [details] [review]
full patch with launchd support

this is a cleaned-up version of the previous patches, with a new algorithm for doing the dbus-bus.c code which should be safer as far as fallbacks, and a bit saner code-wise, as I learn more about C.  ;)

Summary:

Add support for launchd (http://developer.apple.com/macosx/launchd.html)
Launchd is superficially like inetd in that it controls all resources for
system-wide and session-level daemons.

In the case of this dbus implementation, launchd allocates and listens on
a socket, dynamically launching dbus when a client tries to access the
socket for the first time.  When dbus-daemon is started, it asks launchd
for the socket file descriptor, and then initializes itself using that
pre-existing FD.  Note that this only occurs if <listen> in session.conf
is set to "autolaunch:" -- it is still possible to us DBus on Mac OS X
in the normal UNIXy way by specifying a tcp: or unix: session address.

From the client-side, this code will do a number of things to attempt to
auto-determine the socket.  If DBUS_SESSION_BUS_ADDRESS is set, the client
will connect normally, just like any other unix client.  If it is not, it
checks for the environment variable DBUS_LAUNCHD_SESSION_BUS_SOCKET and
crafts a unix: socket URL from it.  If that is not set, it asks launchd
(through the 'launchctl' command) for the socket directly, and crafts a
unix: socket URL from it instead.  It then continues normally.
Comment 9 Colin Walters 2008-02-12 13:33:16 UTC
You said this is a full patch, but I don't see any functions that call _dbus_server_new_for_launchd_fd? 

> If it is not, it checks for the environment variable
> DBUS_LAUNCHD_SESSION_BUS_SOCKET and crafts a unix: socket URL from it.

I understand the launchd protocol for retrieving the unix socket path; but under what conditions is this environment variable set?

More generally, can you describe how you see DBus being deployed on OS X?  Is this patch something that would be used if DBus was installed by fink/macports?

+++ b/dbus/dbus-server-launchd.c
@@ -0,0 +1,136 @@
+/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */
+/* dbus-server-win.c Server implementation for WIN network protocols.

Looks like copy/paste leftover.

+ * Licensed under the Academic Free License version 2.1

Can you switch to the MIT license?  We're in the process of trying to relicense DBus.  Here's a sample new header from dbus-python:

http://gitweb.freedesktop.org/?p=dbus/dbus-python.git;a=blob;h=3a551ea5ea1212460276dbabaa7505e471623cc9;hb=dff98456995c37d964eb32a7de7ca718fc3d48d7;f=dbus/connection.py

Keep your copyright, and use the header to match the one in that file.
Comment 10 Benjamin Reed 2008-02-12 13:41:10 UTC
Created attachment 14290 [details] [review]
whups -- proper full patch

Not sure how I got the wrong one in there; I had gotten a little mixed up in my git stuff and redid everything a few times before I figured out what I was doing wrong.

This should be the proper patch.
Comment 11 Benjamin Reed 2008-02-12 13:46:29 UTC
Created attachment 14291 [details] [review]
relicense as MIT

relicense the git bits to MIT
Comment 12 Benjamin Reed 2008-02-12 13:54:01 UTC
OK, to answer your other question; yes, it is reasonable that these patches could be included by fink or macports, although only one would win when installing the LaunchAgent, presumably.  :)

They also don't get in the way of the existing DBUS_SESSION_BUS_ADDRESS so if you would like to compile in x11 autolaunch, it is still possible to use it instead by using the dbus-launch-x11 stuff and have the exact same behavior as now.

The way it works is, the .plist file gets put into a system location where it will automatically register with launchd; it tells launchd "I have this program called dbus-daemon which needs a socket; provision one, associate the variable DBUS_LAUNCHD_SESSION_BUS_SOCKET with this user's session, and then launch dbus-socket".  If the dbus-session config sees "autolaunch" as the address, it will ask launchd for the variable, and start listening for dbus messages on the socket it refers to, just like a normal dbus session would.

From the client side, if DBUS_SESSION_BUS_ADDRESS exists, it just uses it, it assumes that you're asking it for that session bus.  Otherwise, it checks for DBUS_LAUNCHD_SESSION_BUS_SOCKET and if it exists, constructs a normal session bus address from it and continues on normally.
Comment 13 Colin Walters 2008-03-13 07:37:18 UTC
static void
launchd_init_environment() {

Let's move this into dbus-sysdeps.c as say _dbus_unix_launchd_initialize_environment.

  launchd = popen("launchctl getenv DBUS_LAUNCHD_SESSION_BUS_SOCKET", "r");

Ick!  Is there really no proper API for this?

I'm not sure if the diff viewer is just off, but the indentiation in this function seems really off (does it have tabs?).  The DBus style is "gnu"; 2 spaces, no tabs.

On the configure change:
http://bugs.freedesktop.org/attachment.cgi?id=14290&action=diff#a/configure.in_sec4
I kind of like the idea of changing the whole default to something like "autosession", which is a platform-specific definition.  But let's leave that aside for now.

Comment 14 Benjamin Reed 2008-03-13 08:34:05 UTC
(In reply to comment #13)

> Let's move this into dbus-sysdeps.c as say
> _dbus_unix_launchd_initialize_environment.

ok

>   launchd = popen("launchctl getenv DBUS_LAUNCHD_SESSION_BUS_SOCKET", "r");
> 
> Ick!  Is there really no proper API for this?

On 10.5, yes; on 10.4, no, unfortunately.  I can add some header-checking stuff and get it to autodetect.

> I'm not sure if the diff viewer is just off, but the indentiation in this
> function seems really off (does it have tabs?).  The DBus style is "gnu"; 2
> spaces, no tabs.

Hm, OK; is there a good tool for reformatting it?

> On the configure change:
> http://bugs.freedesktop.org/attachment.cgi?id=14290&action=diff#a/configure.in_sec4
> I kind of like the idea of changing the whole default to something like
> "autosession", which is a platform-specific definition.  But let's leave that
> aside for now.

Yeah, it seems like it should be genericized more.  But kind of outside of the scope of this particular feature addition.  =)
Comment 15 Colin Walters 2008-03-13 08:42:20 UTC
(In reply to comment #14)

> On 10.5, yes; on 10.4, no, unfortunately.  I can add some header-checking stuff
> and get it to autodetect.

If you need to do that to work on 10.4 that's OK, let's keep it as is then.  It's probably worth a comment though.

> Hm, OK; is there a good tool for reformatting it?

I think to do it safely the best way is from your editor.  If you run the whole file through something like /usr/bin/indent it'll likely cause spurious changes in other sections.
 
Comment 16 Colin Walters 2008-03-13 08:53:15 UTC
+#define POPEN_MAX_LENGTH MAXPATHLEN + 2

This seems fairly arbitrary...the maximum length here isn't related to popen(), it comes from the data we're trying to read from launchctl.  I'd call this #define DBUS_LAUNCHD_MAX_OUTPUT_LINE instead.

Also, instead of fgets and a hardcoded line length limit, we could use _dbus_read which can write directly into a DBusString.  You'd have to call fileno() on the FILE object to get the fd for that.

Dealing with subprocesses is complex, and in C it's several times more painful.

Comment 17 Tanner Lovelace 2008-05-15 20:26:10 UTC
Created attachment 16569 [details] [review]
Updated patch addressing comments

Ben's been rather busy with a bunch of other stuff and since he took some of my code for the original basis of his patch, I'm trying to pick up where he's left it and move this bug forward.  

I've attached a new patch that addresses some of the previous comments.  Specifically from comment #13, I've moved launchd_init_environment() into dbus-sysdeps-unix.c and renamed it _dbus_unix_launchd_initialize_environment.  Also, I've tried to clean up the formatting to match the gnu style.  From comment #16, I've changed the define to be DBUS_LAUNCHD_MAX_OUTPUT_LINE and switched from fgets to _dbus_read.  

As far as I can tell, this addresses the current open questions in this bug.  If there is anything I have missed, please let me know!  Otherwise, I'd love to have feedback on this patch and I'd love to get this into dbus.  I've tested it on my OS X setup and it seems to work fairly well for me.

Thanks very much.
Comment 18 Benjamin Reed 2008-05-27 14:07:05 UTC
I've been testing this with new KDE/Mac packages and everything works here.  Is there anything left keeping this from making it into dbus proper?
Comment 19 Colin Walters 2008-05-28 20:07:55 UTC
I took a quick look at this and noticed some cleanups that could be done.  I started reworking it myself, and will hopefully post shortly.  If I don't this week ping me again ;)
Comment 20 Colin Walters 2008-06-11 13:23:01 UTC
Created attachment 17056 [details] [review]
lots of refactoring

Ok, as promised here's a new patch.  This one makes a number of changes. The general idea is that instead of hardcoding unix:tmpdir= in the session.conf file, we create a new scheme default:type=session which says to use a builtin platform-specific method.

We add some new platform-specific functions for looking up the session address, instead of calling a special _launchd function.

I stubbed these functions out in dbus-sysdeps-win.c too, so the code to handle the session bus on Windows should be just filling in those.

Also instead of a custom function to popen("launchd"), we already had code in dbus-sysdeps-unix.c for parsing the output of dbus-launch, so I refactored it into a general function for reading a line of output from a subprocess, and then made the launchd code and the autolaunch code use it.

A random thought that just occurred to me - we may need to strip the carriage return off in the launchd function.

Review and most particularly testing on a Mac (I don't have one) would be appreciated!

I'm hoping we can finish up the Mac and Windows bits and move closer to DBus everywhere.
Comment 21 Colin Walters 2008-06-11 13:41:10 UTC
Created attachment 17058 [details] [review]
also remove obsoleted function

This additional patch deletes a function from the old patch which I accidentally left in.
Comment 22 Colin Walters 2008-06-11 15:04:20 UTC
Created attachment 17059 [details] [review]
add autolaunch test case

This patch adds a test for autolaunch, which will ensure we run through this new code path.  This caught a bug in the previous patch.
Comment 23 Colin Walters 2008-06-11 15:05:37 UTC
Created attachment 17060 [details] [review]
Fix argument ordering

The argument ordering was wrong in the previous patch.  This also renames "address" to "result".
Comment 24 Benjamin Reed 2008-06-13 06:39:20 UTC
haven't had a chance to test it, but at first glance, looks good
Comment 25 Havoc Pennington 2008-07-11 20:01:19 UTC
Comment on attachment 17056 [details] [review]
lots of refactoring

I think it would be a good bit less opaque to just put the default for each platform (e.g. unix:tmpdir) in session.conf, changing it via configure.ac if it's different per-platform. Then you can read the config file and see what the default is.

default:session is wrong in pretty much the same way keying behavior off the bus type is wrong: the config file is supposed to define a bus type, that's at least 75% of the point of the config file. There isn't much site-local config to dbus.

Maybe starting from scratch you'd do it differently and hardcode the session vs. system behaviors in C, but I think it's just a mess to half-way-sort-of-sometimes change this design approach.

-  <listen>unix:tmpdir=@DBUS_SESSION_SOCKET_DIR@</listen>
+  <!-- Use the platform-specific default from the code -->
+  <listen>default:type=session</listen>

Another suck here is that it's now unclear how I'd change that socket directory. Which is maybe useful at least for embedded uses, which seem popular on dbus list.

+      else if (supported && !retval)
+        _dbus_warn ("Dynamic session lookup supported but failed");

Would be good to have some error message as to why it failed.

+DBusServer*
+_dbus_server_new_for_launchd_fd (const char *socket_key, DBusError *error)


args on separate lines

+#ifdef DBUS_ENABLE_LAUNCHD
+#include "dbus-server-launchd.h"
+#endif

I think it's nicer if this #ifdef is not here (make sure no system headers related to launchd leak into the dbus-server-launchd.h and you won't need this #ifdef)

+if DBUS_ENABLE_LAUNCHD
+LAUNCHD_SOURCES = \
+	dbus-server-launchd.h \
+	dbus-server-launchd.c
+endif
+

Does this break distcheck if you distcheck on a system with no launchd? Or is automake clever enough to figure it out? Don't remember.

+#ifdef DBUS_ENABLE_LAUNCHD
+  else if (strcmp (method, "launchd") == 0)
+    {

You could make _dbus_server_new_for_launchd_fd just return an ENOSYS type thing if no launchd, and then avoid any #ifdef DBUS_ENABLE_LAUNCHD here. Pretty sure there's an existing dbus error name for ENOSYS equivalent.

+#ifdef DBUS_ENABLE_LAUNCHD
+#include <sys/param.h>
+#endif
+

Needs some comment on why launchd requires this header perhaps

+ (strlen(env_var) == 0) 

"*env_var == '\0'" is a a better idiom for this used elsewhere in dbus I think.

+  env_var = _dbus_getenv("DBUS_LAUNCHD_SESSION_BUS_SOCKET");

Isn't it technically a value not a variable? I got a bit confused by that on first reading since I skimmed quickly.

+ // strip the carriage-return

No C++ comments
Comment 26 Havoc Pennington 2008-07-11 20:05:02 UTC
I really would not do that default:session thing, it is just not right. You already know at configure time whether to put unix:tmpdir or launchd: in there. The "symlink" of default:session -> unix:tmpdir just adds code and confusion.

The patch does look like a nice advance in general though btw, and illustrates nicely how win32 should be done.
Comment 27 Colin Walters 2008-07-21 06:52:16 UTC
*** Bug 16791 has been marked as a duplicate of this bug. ***
Comment 28 Havoc Pennington 2008-07-21 08:06:48 UTC
The windows patch on bug #16788 determines the default session bus address in configure, which would be useful here as well.

Other stuff in that patch may also be relevant, such as disabling the system bus.
Comment 29 Colin Walters 2008-10-18 11:56:10 UTC
Havoc, any chance I can convince you on comment #26?  While looking at bug 17970, I wanted to change again how the default session address works.  I really feel that this just works better in code. 

Both the system and session buses are special in that they have platform-specific details that don't make sense to try to wedge into the XML config file and connection syntax.
Comment 30 Havoc Pennington 2008-10-19 10:02:14 UTC
I can be convinced on anything but only if it makes sense ;-)

The *only* purpose of the config file, basically, is to define the session vs. system bus. That's what it does. So having something in the config file that is "do what the session bus should do" is just pretty odd to me, it would imply eventually dropping the config file entirely. Which may have been an OK design decision up front, but it seems like once the current approach is in place, we should be consistent about it. 

The current approach does have advantages, like allowing a custom bus to do anything the standard buses can do, and allowing admins and others to easily understand how the two standard buses work and how they differ, without reading C source. Also there are some occasions where you might want to change the standard buses, e.g. make the session bus listen on TCP. It's much easier to change the default if the default is visible in the file rather than hardcoded.

That's the sort of big-picture issue, but the smaller-picture issue is that we already do set a bunch of stuff in the config file from configure, so I just don't see why it's hard to do that based on the platform? It's just another AC_SUBST.

For bug 17970, I would see adding the uid much as we do the guid, where it's inserted at runtime as an extra ignorable field in the address. This would mean it doesn't have to be in the config file. guid is already a dynamic field in the bus address that isn't in the config file, and uid could be done the same way.

Why specifically do you think default:session works better in the code? 
Comment 31 Colin Walters 2008-10-19 10:24:04 UTC
Can't you use the config files to define custom buses?  Not that many people do this probably.

I have two arguments; let me rephrase concisely:

* Administrators can't (or should not) change it or things will explode; thus, having it in code makes more sense and moves us 0.0001% closer to the dream of an empty /etc
* What we would be putting in the config file ends up being very platform-specific and opaque; for OS X it would talk about "launchd:session", for Windows I'm not sure (win32com:session?)

On your suggestion of just always appending the uid - this would be platform-specific (Windows has user ids but...), and I'd argue we only really want to do it for the session bus.  Right now the GUID stuff is in DBusServer's generic code.
Comment 32 Colin Walters 2008-10-19 10:30:51 UTC
Instead of putting "default:session" in the config file, we could have just <listen></listen> mean "default".  Or possibly eliminate the listen node entirely but that patch seemed somewhat tricky when I looked at it last.
Comment 33 Havoc Pennington 2008-10-19 11:18:08 UTC
Do launchd:session or win32com:session need to be opaque? Glancing at the original launchd patch, it looks like it already does something like "launchd:key=foo" where the key can be anything, so you could have launchd:key=mycustombus for example. That seems analogous to unix:path=/tmp/foobar right? I think that's a lot more clean and flexible than hardcoding the whole thing so that simply saying "launchd" implies a particular key.

Similarly with win32, say you did single-instance using the trick of looking up a window by class, you could have an address like win32window:class=whatever ; or if using COM, you could have win32com:moniker=whatever

This seems self-documenting for both us and sysadmins, it allows custom buses, it's consistent with how the code already works ...

Here's how I think about a "symlink" from the word "session" in config file to a hardcoded #define in the C code: It would be like having a gconf setting where the default value was "theDefaultValue" and then in the C code doing:

 if (!strcmp(value, "theDefaultValue")) { value = ACTUAL_DEFAULT; } 

I mean, it works, but it's just extra code... why not use your actual default in the first place?

Re: the uid, I'm not sure that's the correct fix for the other bug (see other bug for discussion), but if it is, appending it always for all addresses seems harmless. But if not, you could always have a config file parameter for whether to append it and then again it would document this session vs. system difference and allow custom buses to toggle it.

That's the small picture... in the big picture, I guess one thing about /etc is that the dbus "config file" is not really a config file ... it's more like the *defaults* in a gconf .schemas, than like a gconf setting ... so like gconf schemas defaults, it can be clean and useful to have the default in a file instead of in C code, but also like gconf schemas defaults, the dbus config defaults should probably be in datadir not sysconfdir, and the app more or less breaks if the defaults aren't present. Debian does move gconf schemas to datadir.

There *are* a few things in the config file that can make sense to change, such as TCP listening, or if we ever supported e.g. Kerberos authentication, enabling that would make sense. And while the security policies and limits in theory should not need changing, it is nice that they can be fixed or played with without a recompile, imo.

It would be a bit more correct to put the default config file in datadir, and then the ".d" directories in sysconfdir, perhaps. In any case that's how I've always thought about it, even though that isn't how it's currently done.

Bottom line I think despite the name, the dbus config file is a required part of the software, like a .glade file or .schemas, rather than a config file.
Comment 34 Jonas Bähr 2009-01-09 18:47:26 UTC
Created attachment 21849 [details] [review]
cleanup and fixes, removed "default:" address type

Hi,

I've ported this patch to the current master's HEAD (only context changes) and made it actually work (Colin's refactoring broke it). In addition I addressed all of Havoc's comments appart from the "if DBUS_ENABLE_LAUNCHD; LAUNCHD_SOURCES = ... since the autotools seem clever enough to include the conditional files in the release. Therefore I haven't seen a reason to refactor the #ifdef DBUS_ENABLE_LAUNCHD in dbus/dbus-server-unix.c

Since there seem some further discussions needed wether the default listen address should be hardcoded in c or rather be in the config file, I removed that bit too (fall back to Benjamins original solution). Personally I'm with Havoc on this one, but maybe you could discuss that independently of this patch.

It would be really nice if this feature could be integrated as soon as possible since it solves a lot of issues with dbus on Mac OS X. Non-X11 apps, notably KDE-4, are barley usable without this.

Regards,
Jonas
Comment 35 Havoc Pennington 2009-01-10 12:20:40 UTC
Thanks Jonas, 

As a double-check, be sure to 'make check' and be sure it passes, and try building the docs ("doxygen Doxyfile" iirc) and see if there are doxygen warnings related to the patch.

+LIBTOOLIZE=libtoolize
+
+($LIBTOOLIZE --version) < /dev/null > /dev/null 2>&1 || {
+        # at least macports installs it as glibtoolize
+	      LIBTOOLIZE=glibtoolize
+}
+

I would expect if macports is going to be weird about this, it would also take care of setting LIBTOOLIZE=glibtoolize when calling configure, to override?

This just doesn't seem like it could possibly make sense, to patch every configure in macports to have something like that in it.

Always using a variable LIBTOOLIZE in our stuff makes sense to me but knowing about "glibtoolize" does not make sense to me. Nothing else will know about glibtoolize either.

+DBusServer* _dbus_server_new_for_launchd_fd (const char *socket_fd, DBusError  *error);

Should be formatted with 1 arg per line, see other headers.

Why is it called "fd" here, but in the launchd:key=session it's called "key" ?

+#ifdef DBUS_ENABLE_LAUNCHD
+#include "dbus-server-launchd.h"
+#endif

The #ifdef isn't needed here, I think? No launchd header should ever be in dbus-server-launchd.h

+#ifdef DBUS_ENABLE_LAUNCHD
+  else if (strcmp (method, "launchd") == 0)

Hmm, in fact: we could trivially de-ifdef this and also remove the Makefile.am conditional. Just move the #ifdef all to inside dbus-server-launchd.c:

#ifdef ENABLE_LAUNCHD
/* most of dbus-server-launchd.c
#endif /* ENABLE_LAUNCHD */

DBusServer* _dbus_server_new_for_launchd_fd (const char *socket_fd, DBusError  *error) 
{
#ifdef ENABLE_LAUNCHD
/* real body */
#else  /* ENABLE_LAUNCHD */
dbus_set_error(error, "launchd: addresses don't work on this platform");
return NULL;
#endif
}

This is a bit cleaner since there's less ifdef stuff, and thus it's harder for someone to break the ifdef case they aren't testing.

+                            const char * const *argv,

The consts here just create a need to cast; const on char** pretty much doesn't work in C, it's busted by design. better to just skip it usually. (or put the const only on the outside, "const char** argv"

+          progname = strrchr (progpath, '/');
+          if (progname != NULL)
+            execvp (progname+1, (char **) argv);

I think there may already be a utility function in dbus-string.h that pulls out the last element in a path. If not, there should be. See HACKING iirc, it discusses our aversion to C strings and pointer math.

+  /* from launchd's environment we get only the path to the socket */
+  if (!_dbus_string_append (address, "unix:path="))
+  {

braces in the wrong column in this function

Looking good, thanks
Comment 36 Jonas Bähr 2009-01-10 17:06:31 UTC
Created attachment 21875 [details] [review]
new full patch, addressing havoc's comments

I fixed one Doxygen warning, the others are not related to the patch.

The LIBTOOLIZE stuff is just to make autogen.sh work. Once there is a configure it's not needed any more. It's however strange that macports call it glibtoolize, but if you google for "macports libtoolize" you'll find tons of fixes like that...

I renamed _dbus_server_new_for_launchd_fd to _dbus_server_new_for_launchd_key. It seems like a left over from the original patch.

I removed the #ifdef ENABLE_LAUNCHD in favor of your solution. It's indeed nicer, now one get an error message about the unsupported launchd address type.

In order to remove the need to cast I changed "const char * const *argv" into "char * const *argv". This argument is passed to execv, which expects a "char * const argv[]". In addition, in every use case of the concerned function, argv is filled wich "const char*", so in my eyes changing it to "char **" is not helpfull.

I removed the need for the pointer math. The concerned code tried to execute a program which was given with full path. If it failed, only the name was looked up using $PATH, which seem a very strange and dangerous behaviour.

The indention is fixed.
Comment 37 Havoc Pennington 2009-01-11 08:40:27 UTC
Revised patch looks good to me, thanks!

Colin maybe you want to look it over? (I'm not sure what branches you'd want this on if it's ok.)
Comment 38 blb 2009-01-13 19:37:56 UTC
FYI, Apple's Xcode also installs libtoolize as glibtoolize; /usr/lib/libtool is actually Apple's libtool, so the Gnu tools are /usr/bin/glibtool and /usr/bin/glibtoolize.
Comment 39 Jonas Bähr 2009-01-16 08:21:05 UTC
The current patch has a problem with auto-launching services. Dbus clients started by the dbus-daemon can't find the bus.
Example: gconf-editor sends a message to gconfd-2. As this service is not running yet, dbus-daemon starts it. Here gconfd-2 can't find the bus address. It works find if gconfd-2 is started manually.

In the Macports community we're working on a solution, see http://trac.macports.org/ticket/17950
Once this issue is solved I'll poste a new patch.
Comment 40 Jonas Bähr 2009-01-19 18:18:33 UTC
Created attachment 22109 [details] [review]
Next version of the patch, now with README and launching services fixed

Hi,

This new version of the patch fixes a problem where services, spawned by dbus-daemon, can't find the bus. An other thing is that "launchd:"-addresses can now also be used to connect to a bus, the previous version allowed them only to start a server on it.

In addition I added a README.launchd which describes how this stuff works.
Comment 41 Benjamin Reed 2009-02-18 07:25:59 UTC
I've integrated this patch into the Fink version of dbus as well, and it works perfectly, great changes!

One issue I'm wondering how to deal with.  GNOME's gnome-session does checks for DBUS_SESSION_BUS_ADDRESS and tries to dbus-launch if it doesn't find it.  Obviously this is relying on a detail of dbus's implementation and should not really be done, but how, instead, should they be doing sanity-checks for dbus?

I want to open a bug against gnome-session, but I'm not sure what to tell them is the "correct" way to handle this.

Should we maybe add a variable to the .pc file or is that just adding a hack upon another hack?
Comment 42 Colin Walters 2009-02-18 07:34:24 UTC
(In reply to comment #41)
> I've integrated this patch into the Fink version of dbus as well, and it works
> perfectly, great changes!
> 
> One issue I'm wondering how to deal with.  GNOME's gnome-session does checks
> for DBUS_SESSION_BUS_ADDRESS and tries to dbus-launch if it doesn't find it. 
> Obviously this is relying on a detail of dbus's implementation and should not
> really be done, but how, instead, should they be doing sanity-checks for dbus?
> 
> I want to open a bug against gnome-session, but I'm not sure what to tell them
> is the "correct" way to handle this.

Hi, I did the gnome-session patch.  For better or worse the dbus specification:
http://dbus.freedesktop.org/doc/dbus-specification.html
actually specifies the existence of the DBUS_SESSION_BUS_ADDRESS environment variable.  It also talks about an X property though I don't know if that's actually used/implemented.

I haven't looked at the new launchd patch yet in detail but my suggestion here is that this environment variable should exist in the OS X session bus, even if it's just "launchd:fd=5" or something.
Comment 43 Benjamin Reed 2009-02-18 07:59:30 UTC
Ick, it is part of the spec...  Well, the problem is that launchd-launched stuff does not guarantee that the environment variables get put into the user's actual environment, only the launchd environment.

On 10.5, if you start a new application after launchd has set up the environment and started DBus, you will get those, but if you're on 10.4, or if you've just used launchctl to enable dbus, it won't be in your environment.  :(

I'm not sure there is a way to solve that in a repeatable way without saying it only supports 10.5.

Perhaps we could put a variable in dbus's .pc file to denote it has launchd support, and work around it in gnome-session?
Comment 44 Colin Walters 2009-02-18 09:26:08 UTC
(In reply to comment #43)
> Ick, it is part of the spec...  Well, the problem is that launchd-launched
> stuff does not guarantee that the environment variables get put into the user's
> actual environment, only the launchd environment.
> 
> On 10.5, if you start a new application after launchd has set up the
> environment and started DBus, you will get those, but if you're on 10.4, or if
> you've just used launchctl to enable dbus, it won't be in your environment.  :(

Hm, OK I see the problem with environment variables.

> I'm not sure there is a way to solve that in a repeatable way without saying it
> only supports 10.5.
> 
> Perhaps we could put a variable in dbus's .pc file to denote it has launchd
> support, and work around it in gnome-session?

Yeah, that sounds fine.  If you file a gnome-session bug CC me please.
Comment 45 Jonas Bähr 2009-02-19 05:46:53 UTC
(In reply to comment #42)
> (In reply to comment #41)
> > I've integrated this patch into the Fink version of dbus as well, and it works
> > perfectly, great changes!
> > 
> > One issue I'm wondering how to deal with.  GNOME's gnome-session does checks
> > for DBUS_SESSION_BUS_ADDRESS and tries to dbus-launch if it doesn't find it. 
> > Obviously this is relying on a detail of dbus's implementation and should not
> > really be done, but how, instead, should they be doing sanity-checks for dbus?
> > 
> > I want to open a bug against gnome-session, but I'm not sure what to tell them
> > is the "correct" way to handle this.
> 
> Hi, I did the gnome-session patch.  For better or worse the dbus specification:
> http://dbus.freedesktop.org/doc/dbus-specification.html
> actually specifies the existence of the DBUS_SESSION_BUS_ADDRESS environment
> variable.  It also talks about an X property though I don't know if that's
> actually used/implemented.
> 
> I haven't looked at the new launchd patch yet in detail but my suggestion here
> is that this environment variable should exist in the OS X session bus, even if
> it's just "launchd:fd=5" or something.

actually, with the latest version of the patch it should be possible to export an environment variable with this content:
DBUS_SESSION_BUS_ADDRESS="launchd:env=DBUS_LAUNCHD_SESSION_BUS_SOCKET"

But why does the spec forces the existence of an env var (and even worse, an X11 property) if there is support for getting the session bus via the dbus implementation directly?
Comment 46 Benjamin Reed 2009-02-19 06:28:37 UTC
(In reply to comment #45)
> actually, with the latest version of the patch it should be possible to export
> an environment variable with this content:
> DBUS_SESSION_BUS_ADDRESS="launchd:env=DBUS_LAUNCHD_SESSION_BUS_SOCKET"

D'oh, I'm an idiot.  Yeah, that should work fine.  That said, the latest patch is working flawlessly here; what's the word on getting it integrated into mainline?

> But why does the spec forces the existence of an env var (and even worse, an
> X11 property) if there is support for getting the session bus via the dbus
> implementation directly?

That's a good question.  :)  Still, it's reasonable to set it manually in the Fink environment, for tools that expect it to work the unix-y way.  Thanks for the pointer.
Comment 47 Havoc Pennington 2009-02-20 16:32:10 UTC
The spec doesn't force DBUS_SESSION_BUS_ADDRESS to exist, just documents it as one way you can find the bus, if it's present. Programs should just be doing dbus_bus_get() and see if it returns non-null.

I don't really remember the story with gnome-session, but looking at what it's doing, I think it's just wrong. If there's no session bus, imo gnome-session should do something like

g_printerr("Your X setup is not correctly configured\n"
"dbus-daemon should be launched and provided to any new\n"
"X sessions by the startup scripts\n");
exit(1);

Oh, maybe I remember - is the issue that dbus_bus_get() *does* return a bus, it's just an autolaunched bus so DBUS_SESSION_BUS_ADDRESS doesn't end up existing which breaks the session for non-X apps that won't read the X property?

I guess in effect DBUS_SESSION_BUS_ADDRESS is required since there are non-X apps. In that case, gnome-session should probably print above error if DBUS_SESSION_BUS_ADDRESS is not set, since it effectively requires not only a session bus but also the env variable. But anyway, as far as I'm concerned gnome-session should not launch dbus-daemon any more than it should launch the X server.

On OS X, though, maybe all apps can use launchd (there's no equivalent of non-X apps?) so you might not need this check for the env variable.
Comment 48 Benjamin Reed 2009-02-23 05:38:55 UTC
Created attachment 23209 [details] [review]
new version of the full patch

This is an update to attachment 22109 [details] [review], which fixes a bug in error initialization.

The launchd check in dbus-transport-unix.c was attempting to overwrite &error directly if the launchd socket was in error (to give a better error message).

Once a DBusError is in error, you have to clear it and re-set it.
Comment 49 Benjamin Reed 2009-05-07 05:12:19 UTC
Any chance of getting this integrated into dbus upstream?  It's been pretty well-tested in both macports and fink for a while now, with no ill effects that I'm aware of.
Comment 50 Tanner Lovelace 2009-05-27 08:37:01 UTC
So, hey, is there any chance of getting this patch merged into upstream?  It seems like it's been tested quite a bit now (see Ben's last comment) and it would be great to see this merged.  Thanks much!
Comment 51 Benjamin Reed 2009-05-30 09:59:47 UTC
Created attachment 26307 [details] [review]
final version of the patch

for whatever reason, the last diff I attached had the fixes to the other issue, but was missing the dbus-sysdeps-unix.c changes.

This is the version of the patch that has been in Fink for a couple of months, and has no failure reports from users.
Comment 52 aaron 2009-07-02 14:02:43 UTC
(In reply to comment #51)
> Created an attachment (id=26307) [details]
> final version of the patch
> 
> for whatever reason, the last diff I attached had the fixes to the other issue,
> but was missing the dbus-sysdeps-unix.c changes.
> 
> This is the version of the patch that has been in Fink for a couple of months,
> and has no failure reports from users.
> 

Not to rain on the parade, but I'm having some problems getting this patch to compile with dbus-1.2.14 (osx-10.5)

First, configure says "configure: WARNING: unrecognized options: --enable-launchd". I'm pretty sure this is unrelated, but it looks like 'configure.in' is being patched, but not 'configure'

Second, the configure output, "Init scripts style:       none". This may be technically incorrect, but should that be patched to return "launchd" or "osx" rather than "none"

The biggest problem is that ld is failing on "__dbus_server_new_for_launchd"

Undefined symbols:
  "__dbus_server_new_for_launchd", referenced from:
      __dbus_server_listen_platform_specific in dbus-server-unix.o
ld: symbol(s) not found
collect2: ld returned 1 exit status


I don't know where the put the full compile log, so its on codepad.org for now. http://codepad.org/7E6KUyv8
Comment 53 Benjamin Reed 2009-07-02 14:12:32 UTC
(In reply to comment #52)

> Not to rain on the parade, but I'm having some problems getting this patch to
> compile with dbus-1.2.14 (osx-10.5)
> 
> First, configure says "configure: WARNING: unrecognized options:
> --enable-launchd". I'm pretty sure this is unrelated, but it looks like
> 'configure.in' is being patched, but not 'configure'

Right, the patch is for the git tree, which doesn't have configure, configure is generated in autoconf projects.

Make sure you have reasonably modern versions of libtool, automake, and autoconf installed, and run:

  autoreconf -fvi

It should generate everything for you.
Comment 54 aaron 2009-07-02 19:42:20 UTC
(In reply to comment #53)
> (In reply to comment #52)
> 
> > Not to rain on the parade, but I'm having some problems getting this patch to
> > compile with dbus-1.2.14 (osx-10.5)
> > 
> > First, configure says "configure: WARNING: unrecognized options:
> > --enable-launchd". I'm pretty sure this is unrelated, but it looks like
> > 'configure.in' is being patched, but not 'configure'
> 
> Right, the patch is for the git tree, which doesn't have configure, configure
> is generated in autoconf projects.
> 
> Make sure you have reasonably modern versions of libtool, automake, and
> autoconf installed, and run:
> 
>   autoreconf -fvi
> 
> It should generate everything for you.
> 

Sorry about that, it compiles cleanly with todays git checkout.

I still think it should tell the output from the init check that it is using launchd style init however. Seems like that slot goes to waste.
Comment 55 Benjamin Reed 2009-07-07 12:45:03 UTC
Created attachment 27477 [details] [review]
full patch with Fink fix

Marcus Calhoun-Lopez noticed that I let a Fink-specific define sneak into the patch.  This is an updated version that uses the right define name.

(In the Fink package, I use a different socket name so it doesn't step on a d-bus that someone might have installed system-wise...)
Comment 56 Colin Walters 2009-07-13 08:50:06 UTC
Looking at this patch again now.

Some superficial coding style mismatches; we use GNU, and while I dislike GNU as much as the guy on the street, inconsistency is worse.  

I prefer git-am style patches, and with larger ones split up (where logical/possible) into chunks is best.

This patch breaks make check on my Fedora 11 box, haven't debugged it yet but it's something to do with launching.  

Nevertheless to make some forward progress here I broke out the grouplist change into a separate git commit and pushed that:

commit c71403ddde230378e3beffee21a3d1fe6edc9bce
Author: Benjamin Reed <ranger@befunk.com>
Date:   Mon Jul 13 11:21:08 2009 -0400

    Bug 14259 - Work around broken getgrouplist on MacOS X
    
    We don't get the number of groups, so allocate an arbitrary
    larger array.
    
    Signed-off-by: Colin Walters <walters@space-ghost.verbum.org>

Comment 57 Colin Walters 2009-07-13 09:48:11 UTC
Pushed a second chunk of this patch.  I also stubbed out this change for win32.

commit 6478ec6949c6bb794237b43d03b68f80eba1288c
Author: Colin Walters <walters@verbum.org>
Date:   Mon Jul 13 12:47:19 2009 -0400

    Bug 14259 - Make session address lookup system-dependent
    
    On some platforms such as MacOS X and Windows, we can't depend
    on an environment variable to determine the address of the
    session bus.  Create a sysdep function dbus_lookup_session_address
    which can be filled in with platform-specific code.

Comment 58 Colin Walters 2009-07-13 10:05:36 UTC
Pushed a third chunk of the non-launchd specific bits.  

If you could rebase the patch on top of these 3, fix any remaining style errors, and submit in git-am format with a description of the changes, I think we'd be good to merge it.  Next release though.

commit 6b163e95e7a2318a98c16c0d0944337e38e62efa
Author: Colin Walters <walters@verbum.org>
Date:   Mon Jul 13 13:02:21 2009 -0400

    Bug 14259 - Refactor _dbus_get_autolaunch_address
    
    Split out the process-launching code, which can be reused for
    other applications; in particular, a forthcoming patch to parse
    output from launchd for MacOS X.

Comment 59 Benjamin Reed 2009-07-14 07:10:25 UTC
Awesome, thanks!  I'll try to find some time to update the patches.
Comment 60 Benjamin Reed 2009-07-21 15:26:41 UTC
OK, I've got it updated with the latest changes (picking up DISPLAY from the environment).  I'm going to update the patches momentarily.
Comment 61 Benjamin Reed 2009-07-21 15:27:27 UTC
Created attachment 27892 [details] [review]
make the session bus <listen> tag configurable
Comment 62 Benjamin Reed 2009-07-21 15:27:55 UTC
Created attachment 27893 [details] [review]
the main launchd implementation
Comment 63 Benjamin Reed 2009-07-21 15:28:53 UTC
Created attachment 27894 [details] [review]
look up DISPLAY from launchd if it is not initialized
Comment 64 Benjamin Reed 2009-07-21 15:29:28 UTC
Created attachment 27895 [details] [review]
enable launchd in configure, and tie it into dbus-server-unix
Comment 65 Thiago Macieira 2009-07-22 01:30:49 UTC
Comment on attachment 27892 [details] [review]
make the session bus <listen> tag configurable

On patch 0001:

You're replacing @DBUS_SESSION_SOCKET_DIR@ with @DBUS_SESSION_BUS_DEFAULT_ADDRESS@, which is good. But please remove the AC_SUBST(DBUS_SESSION_SOCKET_DIR) line in configure.in because it's no longer in use.
Comment 66 Thiago Macieira 2009-07-22 01:44:03 UTC
Comment on attachment 27893 [details] [review]
the main launchd implementation

Please remove tabs in your patch. Use spaces-only indentation. Also, please add a space before the opening parenthesis in function calls.

In: _dbus_lookup_launchd_socket: aren't you leaking the DBusString data? Shouldn't you return or fill in a DBusString instead?
Comment 67 Thiago Macieira 2009-07-22 01:51:35 UTC
Comment on attachment 27894 [details] [review]
look up DISPLAY from launchd if it is not initialized

I fail to see the need for this patch. Why do we need DISPLAY on Mac OS X?
Comment 68 Thiago Macieira 2009-07-22 02:05:50 UTC
Comment on attachment 27895 [details] [review]
enable launchd in configure, and tie it into dbus-server-unix

+    if test x$enable_launchd = xyes -a x$have_launchd = xno ; then

Please use && instead of -a:

+    if test x$enable_launchd = xyes && test x$have_launchd = xno ; then
Comment 69 Benjamin Reed 2009-07-22 05:02:25 UTC
Created attachment 27911 [details] [review]
enable launchd in configure, and tie it into dbus-server-unix

> On patch 0001:
> 
> You're replacing @DBUS_SESSION_SOCKET_DIR@ with
> @DBUS_SESSION_BUS_DEFAULT_ADDRESS@, which is good. But please remove the
> AC_SUBST(DBUS_SESSION_SOCKET_DIR) line in configure.in because it's no longer
> in use.
> 
> +    if test x$enable_launchd = xyes -a x$have_launchd = xno ; then
> 
> Please use && instead of -a:
> 
> +    if test x$enable_launchd = xyes && test x$have_launchd = xno ; then

Fixed in this version of the patch.
Comment 70 Benjamin Reed 2009-07-22 05:05:15 UTC
(In reply to comment #67)
> (From update of attachment 27894 [details] [review])
> I fail to see the need for this patch. Why do we need DISPLAY on Mac OS X?

Because Mac OS X is a unix, and does have X11.  Fink's GNOME was having issues with things auto-launched from D-Bus because they wouldn't inherit DISPLAY (which is not necessarily passed on to the D-Bus process) and thus things like GConf would bomb out, causing all kinds of chaos.

I'm fine with leaving this as a Fink-only patch if you want to leave the upstream version a "pure" mac implementation.  I'll be using it either way.
Comment 71 Benjamin Reed 2009-07-22 05:22:31 UTC
Created attachment 27912 [details] [review]
the main launchd implementation

(In reply to comment #66)
> (From update of attachment 27893 [details] [review])
> Please remove tabs in your patch. Use spaces-only indentation. Also, please add
> a space before the opening parenthesis in function calls.

Fixed.  I'd run it through `indent` (which the GNU site said by default gave GNU formatting) but apparently it doesn't fix such things.  I hadn't noticed it missed the indent in the -launchd.c  :P

> In: _dbus_lookup_launchd_socket: aren't you leaking the DBusString data?
> Shouldn't you return or fill in a DBusString instead?

I'm not really sure, I'm not much of a C coder, that stuff was introduced by Tanner I think?  I'll see if I can find someone who understands this better than I to take a look.  :)
Comment 72 Tanner Lovelace 2009-07-22 08:38:52 UTC
(In reply to comment #71)
> 
> (In reply to comment #66)

> > In: _dbus_lookup_launchd_socket: aren't you leaking the DBusString data?
> > Shouldn't you return or fill in a DBusString instead?
> 
> I'm not really sure, I'm not much of a C coder, that stuff was introduced by
> Tanner I think?  I'll see if I can find someone who understands this better
> than I to take a look.  :)

Yes, now that you mention it, it is leaking the DBusString data, and that was indeed probably my fault. My apologies.  Is there any reason not to just return a DBusString?

Comment 73 Tanner Lovelace 2009-07-22 10:56:02 UTC
(In reply to comment #72)

> Yes, now that you mention it, it is leaking the DBusString data, and that was
> indeed probably my fault. My apologies.  Is there any reason not to just return
> a DBusString?

I've given Ben a patch that uses a passed in DBusString instead of returning a char *. (Since he's set up to quickly test it and unfortunately I'm not at the moment.)  Hopefully we can get that issue with these patches fixed quickly.
Comment 74 Benjamin Reed 2009-07-22 15:48:53 UTC
Created attachment 27925 [details] [review]
Tanner's patch for using a DBusString for the socket

Attached is a modified version of Tanner's patch which changes the code to use a DBusString instead of passing around const data.

Thiago, I think this resolves the issues you commented on.  Mind taking another look at the patch?

Thanks!
Comment 75 Thiago Macieira 2009-07-29 05:45:20 UTC
Can you point me to the complete patchset again, in the proper order? There are 6 non-obsolete attachments:

attachment 17059 [details] [review] - unnumbered - looks ok, provided it is passing the test
attachment 27892 [details] [review] - 1/4 - see comment #65: please remove the older AC_SUBST
attachment 27912 [details] [review] - 2/4 - indentation fixed, but there are some trailing whitespace that can be removed (I can remove them upon applying); the memory leak in _dbus_lookup_launchd_socket is still there
attachment 27894 [details] [review] - 3/4 - looks ok to me 
attachment 27911 [details] [review] - 4/4 - addresses comment #65 here; please move the -AC_SUBST
attachment 27925 [details] [review] - 5/7 - fixes the memory leak in _dbus_lookup_launchd_socket, but has coding style violations (missing space before opening parenthesis) and I think now the leak is in _dbus_transport_open_platform_specific: the socket_path string is not freed.

Where are patches 6 and 7?

Please squash patch 5 into patch 2, so that the leaking version of the functions never appear in the history.
Comment 76 Benjamin Reed 2009-07-29 08:10:30 UTC
OK, I went through all the patches and reordered things a bit, including updating the autolaunch test one to apply cleanly.  Patches forthcoming.
Comment 77 Benjamin Reed 2009-07-29 08:11:19 UTC
Created attachment 28160 [details] [review]
make the session bus <listen> tag configurable
Comment 78 Benjamin Reed 2009-07-29 08:11:40 UTC
Created attachment 28161 [details] [review]
the main launchd implementation
Comment 79 Benjamin Reed 2009-07-29 08:11:57 UTC
Created attachment 28162 [details] [review]
look up DISPLAY from launchd if it is not initialized
Comment 80 Benjamin Reed 2009-07-29 08:12:26 UTC
Created attachment 28163 [details] [review]
enable launchd in configure, and tie it into dbus-server-unix
Comment 81 Benjamin Reed 2009-07-29 08:12:55 UTC
Created attachment 28164 [details] [review]
add autolaunch test case
Comment 82 Benjamin Reed 2009-07-29 08:16:01 UTC
OK, all the patches are up-to-date (and in order)

Note that d-bus master won't build right now without the other 2 patches in bug #22888 (which is why the patches are numbered [PATCH X/7] instead of [PATCH X/5]) but they're not related directly to the launchd stuff, just bugs in changes that most people wouldn't have run into on linux.
Comment 83 Jeremy Lainé 2009-10-13 02:51:41 UTC
The _dbus_lookup_session_address_launchd function is buggy, it causes a segfault due to invalid string manipulations.

The fix is quite straightforward: replace "DBusString *socket_path;" by "DBusString socket_path;" and replace references to "socket_path" to "&socket_path".
Comment 84 Mike McQuaid 2010-03-18 15:38:16 UTC
Sorry, could someone give an outline of what still needs to be done for this patch to be merged?
Comment 85 Colin Walters 2010-03-18 18:14:57 UTC
I'm certain the patches need rebasing to master, that'd be a good start.  Sorry about not getting them merged up till now.
Comment 86 Mike McQuaid 2010-04-19 11:19:40 UTC
Ok, I've fixed all the mentioned problems (as far as I can see) and also added an OSX build fix.

I've rebased against master and pushed to gitorious:
http://gitorious.org/dbus-launchd-integration/dbus-launchd-integration/commits/launchd

The patches are in the "launchd" branch and the "master" branch matches the current upstream "master" branch at the time of writing.

The ordering has got slightly screwed on gitorious as I had to modify history but if you pull the repository then you'll see the order of Benjamin's patches correctly.

You probably want to add the remote at:
git://gitorious.org/dbus-launchd-integration/dbus-launchd-integration.git

and then cherry-pick the last 6 patches (the 4 from Benjamin, 1 from Colin and 1 from me).

As far as I can tell, all the tests pass and it all compiles without warnings.

Let me know ASAP if there's anything else I can/should do. I'm very keen on getting this upstream ASAP.
Comment 87 David Zeuthen (not reading bugmail) 2010-04-22 05:18:37 UTC
One thing we will need is something in the spec much like the spec currently says

  The address of the login session message bus is given in the
  DBUS_SESSION_BUS_ADDRESS environment variable. If that variable
  is not set, applications may also try to read the address from the
  X Window System root window property _DBUS_SESSION_BUS_ADDRESS.
  The root window property must have type STRING. The environment
  variable should have precedence over the root window property.

so reimplementations have some idea of what to do here if running on OS X. Thanks.

(FWIW, the Windows porters will also have to add something to the spec here. I had to read the dbus sources this morning when adding support for Win32 to GDBus (a D-Bus reimpl that will be part of the GLib stack) and the SHM method they use to store the bus address.)
Comment 88 Mike McQuaid 2010-04-22 05:55:08 UTC
That's fair enough. However, do the spec changes need to be made before the patches can be included?

I rebased again today an am quite keen on getting these patches merged ASAP.
Comment 89 David Zeuthen (not reading bugmail) 2010-04-22 07:05:29 UTC
(In reply to comment #88)
> That's fair enough. However, do the spec changes need to be made before the
> patches can be included?

I think so and I doubt more than one or two sentences are needed.

Btw, I just played around with this on OS X (using MacPort's D-Bus 1.2.16 packages that includes these patches) and it seems like the way things work, the session bus is _per user_, not per session. I'm saying this because if I ssh into my OS X box, then dbus-send(1), dbus-monitor(1) etc. launched from that ssh sessoin will connect to the session bus that was spawned in my Aqua session. That's probably wrong and needs fixing...

I really don't know how launchd works or if it can even do per-user properly.
Comment 90 David Zeuthen (not reading bugmail) 2010-04-22 07:05:58 UTC
(In reply to comment #89)
> I really don't know how launchd works or if it can even do per-user properly.

Gah, I meant s/per-user/per-session/ here...
Comment 91 Mike McQuaid 2010-04-22 07:39:28 UTC
(In reply to comment #89)
> (In reply to comment #88)
> I think so and I doubt more than one or two sentences are needed.

*Sigh*. I'll try and get onto something here. It's just a bit annoying that by the time I do so I'll need to rebase yet again. Can you give me more explicit guidance on what is missing from the spec here. As far as I'm aware, this doesn't do anything different in relation to the above, it just allows launchd to start D-Bus rather than it being done manually.

> Btw, I just played around with this on OS X (using MacPort's D-Bus 1.2.16
> packages that includes these patches) and it seems like the way things work,
> the session bus is _per user_, not per session. I'm saying this because if I
> ssh into my OS X box, then dbus-send(1), dbus-monitor(1) etc. launched from
> that ssh sessoin will connect to the session bus that was spawned in my Aqua
> session. That's probably wrong and needs fixing...
> 
> I really don't know how launchd works or if it can even do per-user properly.

That's typical for launchd services, you have a user level or a system level service.
Comment 92 David Zeuthen (not reading bugmail) 2010-04-22 09:38:01 UTC
(In reply to comment #91)
> (In reply to comment #89)
> > (In reply to comment #88)
> > I think so and I doubt more than one or two sentences are needed.
> 
> *Sigh*. I'll try and get onto something here. It's just a bit annoying that by
> the time I do so I'll need to rebase yet again. Can you give me more explicit
> guidance on what is missing from the spec here. As far as I'm aware, this
> doesn't do anything different in relation to the above, it just allows launchd
> to start D-Bus rather than it being done manually.

The spec isn't so much concerned about how the bus is started (that's an implementation detail). What the spec needs to specify is where the address is stored and possible what locking is needed.

I'm sorry but I don't know enough about launchd to say exactly what the paragraph should say. I'd be happy to review it though!

> That's typical for launchd services, you have a user level or a system level
> service.

The session bus really is designed to be scoped only for your session; in particular it is not supposed to bleed out to other sessions. I don't think it would be wise to make an exception for OS X but I haven't thought a ton about it... but I do know enough about D-Bus to know that the guarantee "session bus is really limited to the session" is extremely important for some apps on Linux.

One example that comes to mind are notification D-Bus services (akin to Growl on OS X) in GNOME and KDE. For example, this simple D-Bus call

  gdbus call \
    --session \
    --dest org.freedesktop.Notifications \
    --object-path /org/freedesktop/Notifications \
    --method org.freedesktop.Notifications.Notify \
    my_app 42 drive-removable "A Summary" "A body" "[]" "{}" 5000

will put up a notification in the session. Consider the implications if the session bus is now an user bus... then everything starts falling apart. There are many other examples why this is bad.

So if launchd only does user- and system-level then that's a problem and perhaps we need to find another place to set/get the session bus addresses on OS X.

Ugh, I'm sorry I have to be the messenger to tell you that this is a problem with the current patch set... please don't shoot me!
Comment 93 David Zeuthen (not reading bugmail) 2010-04-22 09:47:12 UTC
Lennart pointed me to

http://developer.apple.com/mac/library/technotes/tn2005/tn2083.html#SECLAUNCHDAGENT

which suggests that launchd does support what we need it to do.
Comment 94 Mike McQuaid 2010-04-22 09:52:55 UTC
Hmm, thinking outside the box here a minute. If the launchd support only supported system buses and a session bus had to be started using e.g. dbus-launch, would that be sufficient?
Comment 95 Thiago Macieira 2010-04-22 09:59:23 UTC
I would say that using launchd, even if it means the session is per-user instead of "per session", is the way to go. That's just how things would work on the Mac. 

For example, if you ssh in and run a graphical app, it will show in the display as if you had run from the login session.

PS: the portion of the spec that David quoted is wrong. The address is not saved in a property in the root window in X11, but in a property of the window holding a specific X11 selection.
Comment 96 David Zeuthen (not reading bugmail) 2010-04-22 10:01:18 UTC
(In reply to comment #94)
> Hmm, thinking outside the box here a minute. If the launchd support only
> supported system buses and a session bus had to be started using e.g.
> dbus-launch, would that be sufficient?

I'm pretty sure that it is the _only_ way to do things on OS X - e.g. only starting the session bus when someone actually tries to connect to it.

Remember that the Macports/Fink/etc way of doing things is the odd and special 0.1% case; basically everyone else (99.9%) with an app that uses D-Bus will use libdbus (or gdbus or NDesk DBus) from an appfolder so you can't assume there's a bus at all. That app will then launch its own copy of dbus-daemon (or whatever) typically via dbus-launch.

Hope this helps.
Comment 97 Mike McQuaid 2010-04-22 10:04:57 UTC
(In reply to comment #96)
> Remember that the Macports/Fink/etc way of doing things is the odd and special
> 0.1% case; basically everyone else (99.9%) with an app that uses D-Bus will use
> libdbus (or gdbus or NDesk DBus) from an appfolder so you can't assume there's
> a bus at all. That app will then launch its own copy of dbus-daemon (or
> whatever) typically via dbus-launch.

I'm not sure I agree. Certainly, as a KDE on Mac guy, the end goal for distributing these apps isn't to have a version of D-Bus in every app bundle. There's no reason why D-Bus can't be installed systemwide by a PKG and then used through launchd per-user or per-system.

As Thiago mentioned (and I failed to point out) this is how GUI apps tend to function if you SSH in. In this way, I'd also argue that D-Bus should perform as you saw.

Thiago, what are your thoughts on anything else that needs done before merging?
Comment 98 David Zeuthen (not reading bugmail) 2010-04-22 10:06:47 UTC
(In reply to comment #95)
> I would say that using launchd, even if it means the session is per-user
> instead of "per session", is the way to go. That's just how things would work
> on the Mac. 

Yeah, maybe. It still really rubs me the wrong way that we're calling it a _session bus_ then. Especially if we're adding a new type of bus which is per-user (the socalled 'user bus').

> For example, if you ssh in and run a graphical app, it will show in the display
> as if you had run from the login session.

I'm not sure that's intentional or just a side-effect. Could be either. Could very possibly change in the future. I don't know.

> PS: the portion of the spec that David quoted is wrong. The address is not
> saved in a property in the root window in X11, but in a property of the window
> holding a specific X11 selection.

The spec needs work, yeah...
Comment 99 David Zeuthen (not reading bugmail) 2010-04-22 10:10:11 UTC
(In reply to comment #97)
> (In reply to comment #96)
> > Remember that the Macports/Fink/etc way of doing things is the odd and special
> > 0.1% case; basically everyone else (99.9%) with an app that uses D-Bus will use
> > libdbus (or gdbus or NDesk DBus) from an appfolder so you can't assume there's
> > a bus at all. That app will then launch its own copy of dbus-daemon (or
> > whatever) typically via dbus-launch.
> 
> I'm not sure I agree. Certainly, as a KDE on Mac guy, the end goal for
> distributing these apps isn't to have a version of D-Bus in every app bundle.
> There's no reason why D-Bus can't be installed systemwide by a PKG and then
> used through launchd per-user or per-system.

Sure, you are free to distribute your software however you want. But you still need to handle the case where the app (along with the D-Bus library it happens to be using) is distributed in an AppFolder. That is, until the OS vendor, Apple in this case, starts shipping D-Bus. And I don't think that's going to happen anytime soon :-)

     David
Comment 100 Thiago Macieira 2010-04-22 10:28:19 UTC
(In reply to comment #98)
> (In reply to comment #95)
> > I would say that using launchd, even if it means the session is per-user
> > instead of "per session", is the way to go. That's just how things would work
> > on the Mac. 
> 
> Yeah, maybe. It still really rubs me the wrong way that we're calling it a
> _session bus_ then. Especially if we're adding a new type of bus which is
> per-user (the socalled 'user bus').

I don't think so. It really depends on what you define as a session.

The X11 concept of a session doesn't translate here. On X11, we've tied the concept of session to the environment variable, or to the X11 display (which is BTW also an environment variable).

On Mac, there's no way to tie it to an environment variable easily. There is, AFAIK, no way to add a script to the Aqua initialisation sequence.

Let me put this in another way: is there ANY way on a Mac for the same user to log in twice to the same machine and have separate applications running, without interfering with each other? More specifically: can a user have two sessions?

If a user can't have two sessions, then per user == per session.
Comment 101 David Zeuthen (not reading bugmail) 2010-04-22 10:48:08 UTC
(In reply to comment #100)
> (In reply to comment #98)
> > (In reply to comment #95)
> > > I would say that using launchd, even if it means the session is per-user
> > > instead of "per session", is the way to go. That's just how things would work
> > > on the Mac. 
> > 
> > Yeah, maybe. It still really rubs me the wrong way that we're calling it a
> > _session bus_ then. Especially if we're adding a new type of bus which is
> > per-user (the socalled 'user bus').
> 
> I don't think so. It really depends on what you define as a session.
>
> The X11 concept of a session doesn't translate here. On X11, we've tied the
> concept of session to the environment variable, or to the X11 display (which is
> BTW also an environment variable).
> 
> On Mac, there's no way to tie it to an environment variable easily. There is,
> AFAIK, no way to add a script to the Aqua initialisation sequence.
> 
> Let me put this in another way: is there ANY way on a Mac for the same user to
> log in twice to the same machine and have separate applications running,
> without interfering with each other? More specifically: can a user have two
> sessions?
> 
> If a user can't have two sessions, then per user == per session.

FWIW, I don't think a user can have two sessions today. But that's not really the point. Let me try and put it like this: What happens when Apple introduces a product similar to Microsoft Terminal Services or Linux LTSP? Well, we can always go extend the spec and provide another way to find the bus address on OS X... but.. I'd much rather just get this right first. Keep in mind that getting it wrong is a very bad situation due to the fact that mostly every app using D-Bus (for better or worse) will ship their own copy of code for following the spec (due to Appfolder being used). So it would be nice to get it right from the get go.

Anyway, it is not like I'm not volunteering to fix this and I don't have time to contribute code or research how launchd works. But I'd like to ask, as a minimum, that whatever we end up doing is properly specced out so non-libdbus applications/libraries (such as GDBus and NDesk and so on) can implement the same behavior as libdbus. Thanks.

(Also, even if TS/LTSP exists for OS X it's still a corner case to have multiple session so probably not a biggie at all.)
Comment 102 Jonas Bähr 2010-04-22 23:29:40 UTC
(In reply to comment #87)
> One thing we will need is something in the spec much like the spec currently
> says
> 
>   The address of the login session message bus is given in the
>   DBUS_SESSION_BUS_ADDRESS environment variable. If that variable
>   is not set, applications may also try to read the address from the
>   X Window System root window property _DBUS_SESSION_BUS_ADDRESS.
>   The root window property must have type STRING. The environment
>   variable should have precedence over the root window property.
> 
> so reimplementations have some idea of what to do here if running on OS X.
> Thanks.

Perhaps the readme I wrote for the launchd stuff [1] can serve as a base here. So in short, we could specify this:

  * X11 systems: take the env-var or query X11
  * Launchd systems: Take the env-var or query launchd
  * Win32 systems: take the env-var or query some to-me-unkown windows service

Regarding the session vs. user bus issue, I don't see a problem with the spec here. If a dbus client asks for the session bus and get's an user bus on OS-X it won't be a problem as long as OS-X doesn't separate the sessions for the same user. If, at some point in the future (if at all) OS-X will make this difference, the spec still remains valid. You can query launchd for the session bus and you get one. If this bus is shared between sessions of the same user, it's a bug in the implementation, not the spec.

[1] https://bugs.freedesktop.org/attachment.cgi?id=22109
Comment 103 Mike McQuaid 2010-04-26 09:54:58 UTC
The conversation seems to have dried up here again. Sorry to keep nagging but it's really hard to tell as a D-Bus outside who actually has commit access and can merge these patches.

If there's still stuff that needs done (and I'm not convinced of this, from the conversation) can someone please tell me exactly what. If there's not, can these patches please be included?
Comment 104 Lennart Poettering 2010-07-08 17:41:35 UTC
Here are a few comments:

Regarding: [PATCH 1/7] make session bus <listen> tag configurable

Is this still necessary? Doesn't the --address= command line argument I just commited support for already cover this well enough? (Just wondering, I see nothing bad with the patch otherwise)

[PATCH 2/7] add launchd implementation:

Hmm, for the systemd counterpart i chose to reuse _dbus_server_new_for_socket() but simply use a different way to acquire the sockets. Appears like a much simpler and less intrusive solution to me that just needs minor extensions in dbus/dbus-sysdeps-unix.c.

It's 2010 now, can we get rid of the popen("launchctl getenv") maybe now?

The function declaration in dbus-server-launchd.h is badly indented.

I don't think DBUS_ERROR_LIMITS_EXCEEDED is a good error to throw when you receive the wrong number of fds from launch.

_dbus_server_new_for_launchd() should close the socket it received when it couldn't create a server for it.

The other patches look fine to me.
Comment 105 Andrew SG Rojek 2010-09-24 16:42:46 UTC
Hi.

I've just installed 1.4.0 and have noticed that since 1.2.24,
the pid file is not being removed properly as it was before.

When I restart, I get the following error message in console:

Failed to start message bus: The pid file "/usr/local/gde/var/run/dbus/dbus.pid" exists,
if the message bus is not running, remove this file.

I have to manually remove the pid file before dbus launches
properly creating a new pid file. I've checked the permissions
of the file and it's been set up as dbus:dbus with 0644.

A.
Comment 106 Mike McQuaid 2010-09-29 13:33:25 UTC
(In reply to comment #105)
> Hi.
> 
> I've just installed 1.4.0 and have noticed that since 1.2.24,
> the pid file is not being removed properly as it was before.
> 
> When I restart, I get the following error message in console:
> 
> Failed to start message bus: The pid file
> "/usr/local/gde/var/run/dbus/dbus.pid" exists,
> if the message bus is not running, remove this file.
> 
> I have to manually remove the pid file before dbus launches
> properly creating a new pid file. I've checked the permissions
> of the file and it's been set up as dbus:dbus with 0644.

Sorry, I'm a bit confused. Does this only apply when using the patches or is it a general 1.4.0 issue? Are you on OSX?
Comment 107 Mike McQuaid 2010-09-29 13:47:48 UTC
(In reply to comment #104)
> Here are a few comments:
> 
> Regarding: [PATCH 1/7] make session bus <listen> tag configurable
> 
> Is this still necessary? Doesn't the --address= command line argument I just
> commited support for already cover this well enough? (Just wondering, I see
> nothing bad with the patch otherwise)

I'll take a look at this.

> [PATCH 2/7] add launchd implementation:
> 
> Hmm, for the systemd counterpart i chose to reuse _dbus_server_new_for_socket()
> but simply use a different way to acquire the sockets. Appears like a much
> simpler and less intrusive solution to me that just needs minor extensions in
> dbus/dbus-sysdeps-unix.c.

Ok, I'll look at your approach and see if it can be done less intrusively.

> It's 2010 now, can we get rid of the popen("launchctl getenv") maybe now?

Sorry, I don't understand this, where are you referring to?

> The function declaration in dbus-server-launchd.h is badly indented.

Fixed, rebased, pushed.

> I don't think DBUS_ERROR_LIMITS_EXCEEDED is a good error to throw when you
> receive the wrong number of fds from launch.

Ok, will look at another solution.

> _dbus_server_new_for_launchd() should close the socket it received when it
> couldn't create a server for it.

I can't see where you are referring to, the only place it seems to be getting the socket are when it returns the server, otherwise it's failed to get one and returns NULL?

> The other patches look fine to me.

Ok, cool.

Please feel free to give me any more pointers. This is work I've inherited from other people that aren't really very contactable and I'm desperately trying to get this upstream as every downstream version of D-Bus on OSX uses these patches and I think they should be in D-Bus proper.

Please talk to me as if I'm an idiot, I don't totally understand this stuff. Thanks!
Comment 108 Jonas Bähr 2010-09-30 06:02:32 UTC
(In reply to comment #107)
> Please talk to me as if I'm an idiot, I don't totally understand this stuff.

Please take a look at http://gitorious.org/dbus-launchd-integration/dbus-launchd-integration/blobs/launchd/README.launchd
I've done my best to document how this stuff works, however, may months ago I've given up on pushing these patches further. I've got the impression that upstream isn't really interested and prefers every Mac-distro to maintain the a patched version of dbus :-(
(please prove me wrong, but like many others who worked on this stuff I've given up hope)
Comment 109 Mike McQuaid 2010-12-03 05:46:32 UTC
I'm trying really hard to be a good open-source citizen, I'm a KDE and Homebrew (OSX package manager) developer and I really dislike the fact that literally every downstream version of OSX I've ever seen used on a machine relies on the launchd patches that have been in the bugtracker for more than 2 1/2 years.

I've rebased them and fixed them up to the best of my understanding but I would really like help getting these actually committed. As said, everyone on OSX is using them already and they have minimal or no changes on any other OS (or even unless you enable the specific launchd option with configure).

Could someone please help me out? The previous patch maintainers have now given up and I'd really like to end this long-running fork of D-Bus for OSX.

Thanks!
Comment 110 Mike McQuaid 2010-12-03 13:11:47 UTC
Created attachment 40788 [details]
Git repositories

I've obsoleted the existing patches and removed the first, unnecessary patch, replacing it with a file listing the repositories (pretty pointless but the only way I can find to make them obsolete).

You can get my latest versions rebased on master from the launchd branch on:

GitHub: https://github.com/mikemcquaid/dbus-launchd
Gitorious: http://gitorious.org/dbus-launchd-integration/dbus-launchd-integration/
Comment 111 Mike McQuaid 2010-12-08 15:22:40 UTC
These are now merged! Thanks everyone for your help and rock on! Particular thanks to Ralf Habacker for actually committing them!

These really should have been merged years ago, every downstream version of D-Bus has been using it since this bug report was filed. I realise we're all probably working in our spare time here but it's really demoralising to have to something fester on the bug-tracker this long. I'm the fourth person to try and get these patches merged and arguably I've only succeeded because I've rudely emailed the people with commit access directly. I hope I'm not coming across as rude but I feel there is perhaps a problem with your process that has allowed to this happen and needs pointing out.

Thanks again everyone and good night!


bug/show.html.tmpl processed on Mar 24, 2017 at 04:13:58.
(provided by the Example extension).