(In reply to comment #0)
> if one launches a dbus session bus from systemd

I don't consider this to be a supported thing to do: the systemd transport is currently only suitable for the system bus.

To make it suitable for the session bus, after _dbus_server_listen_platform_specific() in dbus-server-unix.c calls _dbus_listen_systemd_sockets(), instead of passing "systemd:" to _dbus_server_new_for_socket(), it should:

* determine the real address of each fd (probably via getsockname())
* construct a corresponding D-Bus address
  (e.g. "unix:abstract=/tmp/dbus-LI35ckm")
* pass the sockets' addresses to _dbus_server_new_for_socket()
  separated by ";" (e.g.
  "tcp:host=127.0.0.1,port=34653;unix:abstract=/tmp/dbus-LI35ckm"
  if we discover that it was listening on both those sockets)

The whole block with systemd support should probably become Linux-specific (since systemd is), so that the code to turn socket addresses into D-Bus addresses does not need to be portable; the code to turn socket addresses into D-Bus addresses should probably be a Linux-specific utility function.

Patches to do this would be appreciated.

There are likely to be other changes needed to make the systemd transport suitable for the session bus.

(In reply to comment #1)
> I don't consider this to be a supported thing to do: the systemd transport is
> currently only suitable for the system bus.

well, it is one of the only ways i know to get dbus running inside a systemd --user session

> To make it suitable for the session bus, after
> _dbus_server_listen_platform_specific() in dbus-server-unix.c calls
> _dbus_listen_systemd_sockets(), instead of passing "systemd:" to
> _dbus_server_new_for_socket(), it should:
> 
> * determine the real address of each fd (probably via getsockname())
> * construct a corresponding D-Bus address
>   (e.g. "unix:abstract=/tmp/dbus-LI35ckm")
> * pass the sockets' addresses to _dbus_server_new_for_socket()
>   separated by ";" (e.g.
>   "tcp:host=127.0.0.1,port=34653;unix:abstract=/tmp/dbus-LI35ckm"
>   if we discover that it was listening on both those sockets)

thanks for these pointers, i will take a look at implementing this

> Patches to do this would be appreciated.

then you can expect them.

> There are likely to be other changes needed to make the systemd transport
> suitable for the session bus.

as far as i know only the address thing is an issue, i fixed that on my current machine(in a slightly less elegant way) and it now runs fine. (even though i don't think the systemd-dbus activation works correctly, but the dbus activation it self works)

Created attachment 62899 [details] [review]
first patch to detect address from systemd socket

i created a first patch
i still have to add error handling, but want to know whether i am going in the right direction.

(any comments welcome)

(In reply to comment #2)
> well, it is one of the only ways i know to get dbus running inside a systemd
> --user session

Can't you run dbus-launch --exit-with-session (or, after I merge the patches from Bug #39197, dbus-launch --exit-with-x11) with $DISPLAY set, like you would in a non-systemd environment? (See, for instance, /etc/X11/Xsession.d/75dbus_dbus-launch on Debian/Ubuntu systems.)

Or, if you want to avoid dbus-launch, dbus-daemon --address=unix:path=/run/user/smcv/dbus_session_bus or something like that, with a well-known per-user address?

The systemd: transport is only needed for socket activation: it would be nice for socket activation to be possible in sessions, but its absence shouldn't prevent you from using dbus-daemon under systemd.

(In reply to comment #3)
> i still have to add error handling, but want to know whether i am going in the
> right direction.

Yes, this is roughly what I had in mind.

Created attachment 62932 [details] [review]
second version of the patch, including error handling

added error handling and some documentation for the _dbus_append_address_from_socket function

i have tested this only with normal unix sockets, and it works fine.
somebody still needs to test this with inet/inet6 sockets (or at least look at the code i use to format the address, since i never used inet sockets before)

Created attachment 62992 [details] [review]
redone formatting of the ip dresses

forget about my earlier patches, the byte-order for AF_INET and AF_INET6 is all wrong, and there exist functions that do the string formatting for you.

this should be working for inet sockets as well (still not tested)

Comment on attachment 62992 [details] [review]
redone formatting of the ip dresses

Review of attachment 62992 [details] [review]:
-----------------------------------------------------------------

This looks good, but a few bits aren't quite there.

I'd like a regression test for this too - I'll get round to writing one at some point if nobody else does.

The systemd LISTEN_FDS protocol is pretty simple, so a regression test can fake it fairly easily without actually being systemd - it just needs to listen on some sockets, make them not close-on-exec, and run dbus-daemon --address=systemd: --print-address with the LISTEN_FDS environment variable set appropriately. I'd recommend using GLib for the test, like test/dbus-daemon.c does.

::: dbus-1.6.0/dbus/dbus-server-unix.c
@@ +159,4 @@
>            return DBUS_SERVER_LISTEN_DID_NOT_CONNECT;
>          }
>  
> +      if(!_dbus_string_init(&address))

Whitespace nitpicking: if (!_dbus_string_init (&address))

@@ +166,2 @@
>          {
> +          if ( i > 0)

Whitespace nitpicking: if (i > 0)

@@ +171,2 @@
>              }
> +          if(!_dbus_append_address_from_socket (fds[i], &address))

Whitespace nitpicking: "if (", twice

@@ +180,4 @@
>        dbus_free (fds);
>  
>        return DBUS_SERVER_LISTEN_OK;
> +  oom:

I'd call this systemd_oom or something, so it doesn't get mixed up with a more general "oom" label.

If you make _dbus_append_address_from_socket raise a DBusError, I think you'll actually end up with:

systemd_oom:
  _DBUS_SET_OOM (error);
systemd_err:
  /* ... clean up ... */

where you "goto systemd_oom" if the error is not set yet, or "goto systemd_err" if it is set (even if what it's set to happens to be OOM too).

@@ +188,5 @@
> +        }
> +      dbus_free (fds);
> +      _dbus_string_free (&address);
> +
> +      dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);

You've done this twice. If that OOM is hit, that'll be an assertion failure.

You should use _DBUS_SET_OOM (error); to set a non-NULL message.

::: dbus-1.6.0/dbus/dbus-sysdeps-unix.c
@@ +4086,5 @@
>      close (i);
>  }
>  
> +/**
> + * Read the addres from the socket and append it to the string

Nitpicking: "address"

@@ +4091,5 @@
> + *
> + * @param fd the socket
> + * @param address
> + */
> +dbus_bool_t _dbus_append_address_from_socket (int fd, DBusString *address)

Really pedantic layout nitpicking:

dbus_bool_t
_dbus_append_address_from_socket (int         fd,
                                  DBusString *address)

This function can fail for reasons that aren't OOM, so it should return a DBusError (which it sets with _DBUS_SET_OOM or from errno whenever it returns FALSE), just like (e.g.) _dbus_open_socket() does.

@@ +4096,5 @@
> +{
> +  struct sockaddr_storage socket;
> +  struct sockaddr_in *inet_addr = (struct sockaddr_in*)&socket;
> +  struct sockaddr_in6 *inet6_addr = (struct sockaddr_in6*)&socket;
> +  struct sockaddr_un *unix_addr = (struct sockaddr_un*)&socket;

Unfortunately, sockaddr_storage is smaller than sockaddr_un. (Yeah, I know, that defeats the point...)

I think sockaddr code usually benefits from using a union, to avoid all the casts:

typedef union {
  struct sockaddr sa;
  struct sockaddr_storage storage;
  /* can't be called unix because that's often a predefined
   * macro */
  struct sockaddr_un un;
  struct sockaddr_in ipv4;
  struct sockaddr_in6 ipv6;
} AnySockAddr;

AnySockAddr socket;
int size = sizeof (AnySockAddr);
...
getsockname (fd, &socket.sa, &size);

switch (socket.sa.sa_family)
...

@@ +4101,5 @@
> +  char host[71];
> +  char hostip[INET6_ADDRSTRLEN];
> +  int size = sizeof(struct sockaddr_storage);
> +
> +  getsockname(fd, (struct sockaddr*)&socket, &size);

This should probably be error-checked too.

@@ +4122,5 @@
> +      break;
> +    case AF_INET:
> +      if(inet_ntop(AF_INET, inet_addr, hostip, INET6_ADDRSTRLEN))
> +        snprintf(host, 71, "tcp:family=ipv4,host=%s,port=%hu",
> +                 hostip, ntohs(inet_addr->sin_port));

You don't need "host" or snprintf, just use:

   if (inet_ntop (AF_INET, inet_addr, hostip, sizeof (hostip)))
     ...

   if (!_dbus_string_append_printf (address,
       "tcp:family=ipv4,host=%s,port=%u", hostip, ntohs (...)))
     ...

There's no point in using %hu since varargs smaller than int are always "promoted" to the size of an int (the "default promotions"). %u seems less likely to be missing on a rubbish Unix from the 70s or something.

I'd prefer sizeof (hostip) rather than INET6_ADDRSTRLEN (even though they're equal) because the former is obviously correct at a glance.

@@ +4127,5 @@
> +      else
> +        return FALSE;
> +      if(!_dbus_string_append (address, host))
> +        return FALSE;
> +    case AF_INET6:

This case should be #ifdef AF_INET6.

Comment on attachment 62992 [details] [review]
redone formatting of the ip dresses

Review of attachment 62992 [details] [review]:
-----------------------------------------------------------------

*** Tested this patch against dbus-1.6.0 release, systemd-git-v185-39-gb30b426 ***

No unexpected behavior - functioned as intended.

performed dbus-activation test with `xfconfd`, the xfce settings daemon. Killed the session bus, started xfce-settings-editor, dbus session bus ran as intended, xfconfd started as intended. xfconfd had the proper DBUS_SESSION_BUS_ADDRESS in it's /proc/<pid>/environ.

Thumbs up from here, many thanks for writing this patch.

Created attachment 65321 [details] [review]
4th version of the patch

taken into account all issues pointed out by Simon McVittie.

I have not yet got the chance to test this patch, but it compiles fine and should run better than the previous patches.

I hope I finally have the formatting right, and that I will never have to touch dbus code again.

*** Bug 53639 has been marked as a duplicate of this bug. ***

Comment on attachment 65321 [details] [review]
4th version of the patch

Review of attachment 65321 [details] [review]:
-----------------------------------------------------------------

Tested this version of patch on ArchLinux with dbus 1.6.4, under a systemd --user session.

DBus activation of gconfd works as expected after launching 'gconf-editor'.  Without the patch, gconfd does not activate.

Comment on attachment 65321 [details] [review]
4th version of the patch

Review of attachment 65321 [details] [review]:
-----------------------------------------------------------------

This looks fine to me.

(In reply to comment #11)
> Comment on attachment 65321 [details] [review] [review]
> 4th version of the patch
> 
> Review of attachment 65321 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Tested this version of patch on ArchLinux with dbus 1.6.4, under a systemd
> --user session.
> 
> DBus activation of gconfd works as expected after launching 'gconf-editor'. 
> Without the patch, gconfd does not activate.

Sorry to bikeshed on this, but I have two comments:

1) The coding style is inconsistent.  DBus is GNU style, which in particular means space between identifier and parenthesis.
2) Can you please make this a "git format-patch" style patch?  See https://live.gnome.org/GnomeLove/SubmittingPatches for how to write commit messages.  I think you're in the best position to summarize this change.

I *can* fix both of these for you if needed.

Created attachment 68211 [details] [review]
5th version, fixed coding style and did "git format-patch"

Comment on attachment 68211 [details] [review]
5th version, fixed coding style and did "git format-patch"

Committed, thanks for jumping through all of the hoops =)

(I did add a space between the inet_ntop calls and the paren)

Thanks folks. This is great.

Would someone change the state on this bug to reflect the change please? :)

Fixed in git for dbus-1.7.0.

(Auke: I believe anyone can close bugs as FIXED here, you don't need to be a maintainer or anything :-)

Can this be cherry-picked back to 1.6?

(In reply to comment #18)
> Can this be cherry-picked back to 1.6?

The patch applies cleanly, but I am not comfortable with including it in dbus-1.6 upstream. It's more new code than I'd like to add to a stable-branch: stable-branches ought to be stable.

I've been working on user bus activation myself recently, but it appears to be rather unfinished and has a number of awkward corner cases (Bug #61129, <http://lists.freedesktop.org/archives/systemd-devel/2013-February/009119.html> - participation welcome).

If user bus activation is important in a particular distribution (and you can work around the subtleties, for instance by hard-coding that DISPLAY is always :0 on your platform), I would suggest applying this change as a patch, and doing some fairly serious testing/QA on the result.

I should do a dbus-1.7.0 release at some point, which would include this change and be suitable for sufficiently adventurous downstream distributions. My advice would be not to cherry-pick patches or update to a development release unless you are prepared to keep up with development until the corresponding stable branch is reached.

I don't have a great deal of time to work on D-Bus at the moment, so I don't have a fixed timescale for the corresponding stable branch.