Bug 74454

(In reply to comment #0)
> The patch that introduces the unixexec transport in bug 35230 covers the
> parent-side of the unixexec transport, but it's missing the child side -
> making a dbus connection out of input and output file descriptors. Something
> like dbus_connection_open_using_fds() is needed.

I believe the implementations of the child side of unixexec are in systemd and udisks, and are basically "ssh otherhost.example.com socat - UNIX-CONNECT:/var/run/dbus/dbus_system_bus_socket".

I would suggest a simplification: require stdin and stdout to point to the same socket, and require it to be a genuine socket from socketpair() (not a pair of unidirectional pipes).

You don't need a new dbus_connection_open_using_fds(); you could implement a transport called "unix-stdio:" or something, whose semantics are:

* connectable (see http://dbus.freedesktop.org/doc/dbus-specification.html
  for definition)
* not listenable (unless you want it to be; again, see the spec)
* file descriptors 0 and 1 must point to the same SOCK_STREAM socket
* that socket is used to send and receive messages
* file descriptor 2 is only used for warnings etc.

and use dbus_connection_open[_private] ("unix-stdio:", ...) to connect.

The implementation would be quite similar to the systemd: transport.

Yep, that should work too. It would also automatically add support for both sides of unixexec-type connection to dbus bindings built on top of libdbus - Qt, EFL, etc.

Created attachment 94660 [details] [review]
transport: add new unixexec-child transport on Unix

Alright, here's the patch that adds the transport. I've named it "unixexec-child", because it better corresponds to the purpose of it.

The patch was written by following the changes that the original "unixexec" patch did, and doing similar modifications.

Can you take a look at the patch please? It's been 4 weeks.

Comment on attachment 94660 [details] [review]
transport: add new unixexec-child transport on Unix

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

::: dbus/dbus-transport-unix.c
@@ +221,5 @@
> + *
> + * @param error address where an error can be returned.
> + * @returns a new transport, or #NULL on failure.
> + */
> +static DBusTransport*

Whitespace nitpicking: DBusTransport *

@@ +232,5 @@
> +  _DBUS_ASSERT_ERROR_IS_CLEAR (error);
> +
> +  if (!_dbus_string_init (&address))
> +    {
> +      dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL);

Please replace all OOM errors like this with

_DBUS_SET_OOM (error);

which ensures that there is a proper message, even if out of memory.

@@ +515,5 @@
> +  const char *address;
> +
> +  dbus_error_init (&error);
> +
> +  c = dbus_connection_open ("unixexec-child:", &error);

This doesn't test much. In particular, it treats whatever happens to be set as the test's stdin (probably /dev/null or a tty) as if it was a socket.

It would be better to have a separate executable that does (a simplified version of) whatever it is that you want to do with this socket, and actually send data through it.

For instance, doing an equivalent of test/loopback.c or test/dbus-daemon.c using a pair of socat(1) processes (one that is unixexec'd, and one that is the unixexec-child) might make sense?

(In reply to comment #0)
> The patch that introduces the unixexec transport in bug 35230 covers the
> parent-side of the unixexec transport, but it's missing the child side -
> making a dbus connection out of input and output file descriptors.

The high-level question I should have asked to begin with: why do you want this? What functionality do you want, and what are you using it for?

The D-Bus wire protocol has a listening side (like listen() in TCP, with a DBusServer and 0 or more DBusConnections) and a connecting side (like connect() in TCP, with exactly one DBusConnection). They have different roles in the authentication/negotiation handshake, and are not interchangeable.

"unixexec:path=foo" is said to be "connectable" but not "listenable". If you tell libdbus to connect to unixexec:path=foo, it executes the binary "foo" and expects it to connect its stdin/stdout, eventually, via some circuitous route (e.g. ssh and socat), to the listening socket of a DBusServer somewhere.

As far as I can tell, your implementation of "unixexec-child:" is also "connectable" but not "listenable": it is a way for a process to connect to a DBusServer that has been provided on its stdin. With hindsight, this might be what we should have implemented *instead of* unixexec, but it's in the same high-level role as unixexec:.

By contrast, "systemd:" is "listenable" but not "connectable": systemd can tell a DBusServer, like the one built into dbus-daemon, "here's a socket I already opened for you, listen on it please".

So... what processes are you trying to plumb together, and how are they connected?

(In reply to comment #6)
> For instance, doing an equivalent of test/loopback.c or test/dbus-daemon.c
> using a pair of socat(1) processes (one that is unixexec'd, and one that is
> the unixexec-child) might make sense?

No, that doesn't work, because they are both connectable addresses, and neither is listenable. It would have to be a unixexec-child whose stdin (provided by the test harness) is connected to the socket on which a DBusServer listens.

(In reply to comment #7)

> So... what processes are you trying to plumb together, and how are they
> connected?

We need to do high-level IPC between a parent process and a set of child processes. The child process provides to the parent an object with properties, methods and signals. D-Bus is perfect for that because it takes care of converting such an API to/from a bytestream, both on the server (child) and on the parent (client) side.

In our glib/gio-based implementation, when a parent starts a child, it creates a socket pair, and connects the child's standard input and output to one of the sockets. Then we use GIO's stream facility to start a d-bus server on the child side and a d-bus client on the parent side, both using these sockets for tranasport. Then D-Bus IPC can commence using the standard GIO API.

You can see the source code that does this from the links below, and I assure you that it's working great :)

Child side:
https://code.google.com/p/accounts-sso/source/browse/src/gplugind/gsignond-plugin-daemon.c?repo=gsignond#373

Parent side:
https://code.google.com/p/accounts-sso/source/browse/src/daemon/plugins/gsignond-plugin-remote.c?repo=gsignond#480

We would like this to become a standard facility in d-bus implementations and specification. The specification is covering the parent side (the 'unixexec' transport does exactly what we need on the parent side), but is missing the child side - that's what the patch is aiming to address. The plan is to first get the child side support to libdbus and the specification, then contribute both parent and child support to gio, then switch to using that in our project.

Does this make sense?

(In reply to comment #9)
> Then we use GIO's stream facility to start a d-bus
> server on the child side and a d-bus client on the parent side, both using
> these sockets for tranasport.

In that case, a dbus_connection_open() variant is unlikely to be the right "shape" for the child side; sorry if I misled you. dbus_connection_open() takes a connectable address.

I'm tempted to suggest that the right API is something like this:

     dbus_server_listen ("stdin:", &error);

(or "unixexec-child:" if you must, but I would prefer a transport name that documents what it actually does, since this code could be used for things other than the child side of unixexec)

and to have that instantiate a single "listener-side" DBusConnection and pass it to the DBusServer's DBusNewConnectionFunction.

However, I realise this situation is a bit weird, because DBusServer is normally responsible for accept()ing incoming connections, but instead of that, you have a single pre-connected connection - so you would need some sort of trigger for calling the DBusNewConnectionFunction, but only after it has been set up. So yes, maybe

    DBusConnection *dbus_connection_new_server_on_socket (int sockfd);

or

    DBusConnection *dbus_connection_new_server_on_stdin (void);

would be better after all.

GDBus is considerably better-designed for this, because they make it explicit whether a GDBusConnection is trying to be the listening or connecting side, via G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_SERVER and G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT.

The downside of introducing something like bus_connection_new_server_on_stdin() is that all d-bus bindings based on libdbus need to replicate the new function. Whereas by adding new address types things should 'just work' in the bindings. So I'd say 

dbus_server_listen ("stdin:", &error); 

is a better approach for that reason, even though it's somewhat more awkward to use.

By the way, we don't set any authentication flags in our code, so gio goes straight to d-bus communication (authenticating on a private channel between parent and child processes is unnecessary), but I do realize it's not compliant with the spec, and unsupported by libdbus, so that will be changed to use anonymous mechanism.

(In reply to comment #11)
> The downside of introducing something like
> bus_connection_new_server_on_stdin() is that all d-bus bindings based on
> libdbus need to replicate the new function.

Yes; although I think that's only dbus-python, dbus-glib (which is a thing of horror and misery anyway), and QtDBus?

> Whereas by adding new address
> types things should 'just work' in the bindings. So I'd say 
> 
> dbus_server_listen ("stdin:", &error); 
> 
> is a better approach for that reason, even though it's somewhat more awkward
> to use.

If you can make the implementation work, great. My concern is making the implementation work, without adding unexpected reentrancy.

The best I can think of is "when the new-connection callback and the add-timeout callbacks have both been provided to the DBusServer, schedule a timeout 0 seconds in the future, then set a flag to say not to schedule that callback again; when the timeout fires, call the new-connection callback and give it your single connection". Unless you can do better?

> By the way, we don't set any authentication flags in our code, so gio goes
> straight to d-bus communication (authenticating on a private channel between
> parent and child processes is unnecessary), but I do realize it's not
> compliant with the spec, and unsupported by libdbus, so that will be changed
> to use anonymous mechanism.

Thanks. I'd rather not get into whether that's a valid use of D-Bus or not (and as a purely practical thing, it means you can't do the feature-negotiation step, which is part of the authentication handshake).

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/95.