Merge lp:~ted/upstart/dbus-arguments into lp:upstart

[Ted Gould]

Add DBus signal arguments to the Upstart event.

Takes the dbus message and if it has any string arguments adds them to the Upstart event so that other jobs can filter and use them directly.

[James Hunt (jamesodhunt)]

This looks good. A few comments:

- It needs an update to extra/man/dbus-event.7 to explain the ARG%d string arguments.

- I think that we should consider supporting all non-aggregate basic types (except maybe BYTE) since types such as BOOLEAN and the numerics could be equally useful.

- Since we've already released upstart 1.9 including the upstart-dbus-bridge, we need to think about how to handle/break/deprecate the existing 'dbus' event. Allowing the event name to be configurable (lp:~ted/upstart/dbus-configure-event) may be the best way to do it I guess, although the ideal would be to able to support:

  dbus BUS=system SIGNAL=...
  dbus BUS=session SIGNAL=...
  dbus BUS=unix:abstract=/com/ubuntu/upstart-session/1000/2536 SIGNAL=...

... rather than:

  dbus-system SIGNAL=...
  dbus-session SIGNAL=...
  dbus-???? SIGNAL=...

However, if we just switched to that, we'd break any jobs that specify positional values in conditions like 'start on dbus NameAcquired SENDER=...'.

It would lead to slightly more verbose job conditions, but maybe we could change the event to 'message-bug' or similar?:

  message-bus TYPE=dbus BUS=system SIGNAL=...

[Steve Langasek (vorlon)]

On Mon, Jul 08, 2013 at 03:33:30PM -0000, James Hunt wrote:

> - Since we've already released upstart 1.9 including the
> upstart-dbus-bridge, we need to think about how to
> handle/break/deprecate the existing 'dbus' event.

Considering this was pushed to trunk in advance of resolving this multiple
bus question on the grounds that it was useful in its current form and only
needed extended, yes.

> Allowing the event name to be configurable
> (lp:~ted/upstart/dbus-configure-event) may be the best way to do it I
> guess, although the ideal would be to able to support:

> dbus BUS=system SIGNAL=...
> dbus BUS=session SIGNAL=...
> dbus BUS=unix:abstract=/com/ubuntu/upstart-session/1000/2536 SIGNAL=...

I agree that this would be preferred, not just for compatibility but I think
in terms of overall semantics for such an event.

[Ted Gould (ted)]

On Mon, 2013-07-08 at 09:39 -0700, Steve Langasek wrote:

> On Mon, Jul 08, 2013 at 03:33:30PM -0000, James Hunt wrote:
>
> > - Since we've already released upstart 1.9 including the
> > upstart-dbus-bridge, we need to think about how to
> > handle/break/deprecate the existing 'dbus' event.
>
> Considering this was pushed to trunk in advance of resolving this multiple
> bus question on the grounds that it was useful in its current form and only
> needed extended, yes.
>
> > Allowing the event name to be configurable
> > (lp:~ted/upstart/dbus-configure-event) may be the best way to do it I
> > guess, although the ideal would be to able to support:
>
> > dbus BUS=system SIGNAL=...
> > dbus BUS=session SIGNAL=...
> > dbus BUS=unix:abstract=/com/ubuntu/upstart-session/1000/2536 SIGNAL=...
>
> I agree that this would be preferred, not just for compatibility but I think
> in terms of overall semantics for such an event.

You guys are commenting on the wrong branch :-)

Changed it to add the bus name on the event here:

https://code.launchpad.net/~ted/upstart/dbus-configure-event/+merge/172381

Then adjusted the jobs here:

https://code.launchpad.net/~ted/upstart/session-jobs/+merge/172384

I made the bus name configurable because there has generally been talk
in the dbus community before about creating a "user" bus. That would be
one per-user independent of sessions. I figured this would allow us to
have other named buses as well. Perhaps a "unity" bus or something like
that, without having to put the full abstract path in.

[Ted Gould (ted)]

On Mon, 2013-07-08 at 15:33 +0000, James Hunt wrote:

> - It needs an update to extra/man/dbus-event.7 to explain the ARG%d
> string arguments.

Fixed in r1504

> - I think that we should consider supporting all non-aggregate basic
> types (except maybe BYTE) since types such as BOOLEAN and the numerics
> could be equally useful.

Fixed in r1505

[lp:~ted/upstart/dbus-arguments]

1504. By Ted Gould on 2013-07-08

Adding man page for the arguments

1505. By Ted Gould on 2013-07-08

Handle basic types

[James Hunt (jamesodhunt)]

LGTM.

review: Approve

[James Hunt (jamesodhunt)]

Hmm - back-peddling half-a step, we can't technically add one of those session jobs yet as it uses the as-yet-unavailable --event option. In fact, might make more sense to just add those to the Ubuntu package in debian/user-conf/ like we've done for the file bridge.

[Ted Gould (ted)]

On Tue, 2013-07-09 at 16:58 +0000, James Hunt wrote:

> Hmm - back-peddling half-a step, we can't technically add one of those
> session jobs yet as it uses the as-yet-unavailable --event option. In
> fact, might make more sense to just add those to the Ubuntu package in
> debian/user-conf/ like we've done for the file bridge.

I'm confused, what is the --event option? I don't mind moving the
files, but it seems a bit silly to not give them to everyone whether
they use them or not.

[lp:~ted/upstart/dbus-arguments]

1506. By Ted Gould on 2013-07-24

Merge updated session-jobs

[James Hunt (jamesodhunt)]

OK. We've now set precedent by adding other session jobs to extra/conf-session/ so LGTM. Thanks!

review: Approve

=== modified file 'extra/man/dbus-event.7'
--- extra/man/dbus-event.7	2013-07-24 04:45:34 +0000
+++ extra/man/dbus-event.7	2013-07-24 04:45:34 +0000
@@ -11,6 +11,8 @@
 .BI PATH\fR= PATH
 .BI SENDER\fR= SENDER
 .BI DESTINATION\fR= DESTINATION
+.BI ARG0\fR= VALUE
+.BI ARGN\fR= VALUE
 .\"
 .SH DESCRIPTION
 
@@ -26,6 +28,8 @@
 .B stop on
 stanza is modified.
 
+Arguments that are simple types (string, int, etc.) are translated into strings and placed as parameters on the event with the pattern ARG and the number representing the placement in the event prototype.  Parameters that are not a simple type are skipped, with the number being incremented the same.
+
 The
 .B BUS
 parameter is optional and only set if the
@@ -39,7 +43,7 @@
 .\"
 .SH EXAMPLES
 .\"
-.IP "start on dbus SIGNAL=NameAcquired INTERFACE=org.freedesktop.DBus PATH=/org/freedesktop/DBus SENDER=org.freedesktop.DBus"
+.IP "start on dbus SIGNAL=NameAcquired INTERFACE=org.freedesktop.DBus PATH=/org/freedesktop/DBus SENDER=org.freedesktop.DBus ARG0=com.mycorp.foo"
 Start job when a particular
 .I NameAcquired
 D-Bus signal is received.

=== modified file 'extra/upstart-dbus-bridge.c'
--- extra/upstart-dbus-bridge.c	2013-07-24 04:45:34 +0000
+++ extra/upstart-dbus-bridge.c	2013-07-24 04:45:34 +0000
@@ -443,6 +443,7 @@
 	int                 emit = FALSE;
 	DBusPendingCall    *pending_call;
 	DBusError           error;
+	DBusMessageIter     message_iter;
 	nih_local char    **env = NULL;
 	const char         *sender;
 	const char         *destination;
@@ -527,6 +528,97 @@
 		NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, var));
 	}
 
+	if (dbus_message_iter_init (message, &message_iter)) {
+		int current_type = DBUS_TYPE_INVALID;
+		int arg_num = 0;
+
+		while ((current_type = dbus_message_iter_get_arg_type(&message_iter)) != DBUS_TYPE_INVALID) {
+			nih_local char *var = NULL;
+
+			switch (current_type) {
+				case DBUS_TYPE_BOOLEAN: {
+					dbus_bool_t arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%s", arg_num, arg ? "TRUE" : "FALSE"));
+					break;
+				}
+				case DBUS_TYPE_INT16: {
+					dbus_int16_t arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%u", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_UINT16: {
+					dbus_uint16_t arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%d", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_INT32: {
+					dbus_int32_t arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%d", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_UINT32: {
+					dbus_uint32_t arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%u", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_INT64: {
+					dbus_int64_t arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%ld", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_UINT64: {
+					dbus_uint64_t arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%lu", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_DOUBLE: {
+					double arg = 0;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%f", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_STRING: {
+					const char * arg = NULL;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%s", arg_num, arg));
+					break;
+				}
+				case DBUS_TYPE_OBJECT_PATH: {
+					const char * arg = NULL;
+					dbus_message_iter_get_basic(&message_iter, &arg);
+
+					var = NIH_MUST (nih_sprintf (NULL, "ARG%d=%s", arg_num, arg));
+					break;
+				}
+				/* NOTE: Only supporting strings for now, we can consider other
+				   types in the future by extending this switch */
+			}
+
+			if (var != NULL) {
+				NIH_MUST (nih_str_array_addp (&env, NULL, &env_len, var));
+			}
+
+			dbus_message_iter_next(&message_iter);
+			arg_num++;
+		}
+	}
+
 	nih_debug ("Received D-Bus signal: %s "
 		   "(sender=%s, destination=%s, interface=%s, path=%s)",
 		   signal ? signal : "",