|
Description
Benjamin Reed
2008-01-26 11:49:46 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. 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. Created attachment 13959 [details] [review] shell syntax change a minor shell syntax change to make 10.4's /bin/sh happy I'm wondering if this can be done in dbus-launch and not in the bus itself. 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. 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 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 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. 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. 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. Created attachment 14291 [details] [review] relicense as MIT relicense the git bits to MIT 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. 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.
(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. =) (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. +#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. 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. 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? 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 ;) 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. Created attachment 17058 [details] [review] also remove obsoleted function This additional patch deletes a function from the old patch which I accidentally left in. 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. Created attachment 17060 [details] [review] Fix argument ordering The argument ordering was wrong in the previous patch. This also renames "address" to "result". haven't had a chance to test it, but at first glance, looks good 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 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. *** Bug 16791 has been marked as a duplicate of this bug. *** 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. 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. 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? 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. 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. 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.
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 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
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. 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.) 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. 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. 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. 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? (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. 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? (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. (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? (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. 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.
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. 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. 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! 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. (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 (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. (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. 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...) 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> 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. 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. Awesome, thanks! I'll try to find some time to update the patches. OK, I've got it updated with the latest changes (picking up DISPLAY from the environment). I'm going to update the patches momentarily. Created attachment 27892 [details] [review] make the session bus <listen> tag configurable Created attachment 27893 [details] [review] the main launchd implementation Created attachment 27894 [details] [review] look up DISPLAY from launchd if it is not initialized Created attachment 27895 [details] [review] enable launchd in configure, and tie it into dbus-server-unix 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 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 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 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 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. (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. 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. :) (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? (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. 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! 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. OK, I went through all the patches and reordered things a bit, including updating the autolaunch test one to apply cleanly. Patches forthcoming. Created attachment 28160 [details] [review] make the session bus <listen> tag configurable Created attachment 28161 [details] [review] the main launchd implementation Created attachment 28162 [details] [review] look up DISPLAY from launchd if it is not initialized Created attachment 28163 [details] [review] enable launchd in configure, and tie it into dbus-server-unix Created attachment 28164 [details] [review] add autolaunch test case 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. 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". Sorry, could someone give an outline of what still needs to be done for this patch to be merged? I'm certain the patches need rebasing to master, that'd be a good start. Sorry about not getting them merged up till now. 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. 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.) 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. (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. (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... (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. (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! 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. 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 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. (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. (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? (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... (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 (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. (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.) (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 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? 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.
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. (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? (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! (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) 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! 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/ 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! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.