Merge lp:~stgraber/upstart/upstart-make-event-bridge-usable into lp:upstart

[Stéphane Graber]

This branch addresses a few problems with the current implementation of the
event bridge.

== PID file handling ==
In the past, when started with --daemon, nih would try to create a pidfile
under /run, which would obviously fail when running as a user.
The first change is to instead use XDG_RUNTIME_DIR or HOME as the preferred
location for the pidfile.

This brings us to the second problem with the pidfile. A single user may have
multiple upstart instances running at the same time, each of those would
require its own event bridge. So the pidfile can't be unique to the user.

This one was fixed in two ways:
 - upstart-event-bridge.conf no longer uses --daemon, so by default no pidfile
   will be created at all.
 - When called with --daemon, the pidfile now includes the PID of the upstart
   instance, taken from the value of UPSTART_SESSION (last field).

== DBus socket ==
When the event bridge was initially written, we didn't have a private socket
for the user instance, so the bridge was connecting to the DBus session bus.
However, we know have that private socket and no longer use the session bus by
default.

So this branch now changes the default bus from the session bus to the upstart
user session private bus.

[James Hunt (jamesodhunt)]

* extra/upstart-event-bridge.c: main():
  - Looks like user_session_path should be nih_local, whilst path_element should just be 'char *'?
  - If $HOME is not set, we fail to start. That's probably reasonable though as we display a couple
    of errors in that scenario.

review: Needs Fixing

[lp:~stgraber/upstart/upstart-make-event-bridge-usable]

1432. By Stéphane Graber on 2013-01-23

Change which variables are nih_local and which aren't as that was resulting in segfaults when starting with --daemon.

[Stéphane Graber (stgraber)]

> * extra/upstart-event-bridge.c: main():
> - Looks like user_session_path should be nih_local, whilst path_element
> should just be 'char *'?

Done. While testing that change, I also noticed a couple more that shouldn't be nih_local as they were causing segfaults in daemon mode. I didn't spot those earlier because I didn't think of running the daemon under strace to catch any potential segfault on exit of the child.

> - If $HOME is not set, we fail to start. That's probably reasonable though
> as we display a couple
> of errors in that scenario.

Right, the only other thing I could think of was trying to write in CWD but if HOME isn't set, it's fairly likely to be somewhere you can't write to anyway (or shouldn't at least).
The main problem here is that libnih doesn't let you use the daemonise function without also creating a pidfile.

[Dimitri John Ledkov (xnox)]

> > - If $HOME is not set, we fail to start. That's probably reasonable though
> > as we display a couple
> > of errors in that scenario.
>
> Right, the only other thing I could think of was trying to write in CWD but if
> HOME isn't set, it's fairly likely to be somewhere you can't write to anyway
> (or shouldn't at least).
> The main problem here is that libnih doesn't let you use the daemonise
> function without also creating a pidfile.

Spam /tmp/ ?! or /run/upstart/ those should be writable.
Even to $HOMEless mere mortals.

[Stéphane Graber (stgraber)]

/run/upstart shouldn't be user writable. /tmp should work but is really kind of wrong for a pidfile.
So I think I'll stick to the current implementation, which by the way doesn't prevent the daemon from starting at all, it just prints an extra error on the console and doesn't create the pidfile.

[James Hunt (jamesodhunt)]

Agreed. LGTM - merged.

review: Approve

=== modified file 'extra/conf/upstart-event-bridge.conf'
--- extra/conf/upstart-event-bridge.conf	2013-01-10 18:44:12 +0000
+++ extra/conf/upstart-event-bridge.conf	2013-01-23 19:20:39 +0000
@@ -10,7 +10,6 @@
 start on started dbus
 stop on stopped dbus
 
-expect daemon
 respawn
 
-exec upstart-event-bridge --daemon
+exec upstart-event-bridge

=== modified file 'extra/upstart-event-bridge.c'
--- extra/upstart-event-bridge.c	2012-12-19 17:02:33 +0000
+++ extra/upstart-event-bridge.c	2013-01-23 19:20:39 +0000
@@ -1,6 +1,6 @@
 /* upstart
  *
- * Copyright © 2012 Canonical Ltd.
+ * Copyright © 2012-2013 Canonical Ltd.
  * Author: Stéphane Graber <[e-mail]>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -67,11 +67,11 @@
 static NihDBusProxy *system_upstart = NULL;
 
 /**
- * session_upstart:
+ * user_upstart:
  *
- * Proxy to session Upstart daemon.
+ * Proxy to user Upstart daemon instance.
  **/
-static NihDBusProxy *session_upstart = NULL;
+static NihDBusProxy *user_upstart = NULL;
 
 /**
  * options:
@@ -92,13 +92,18 @@
 {
 	char **              args;
 	DBusConnection *     system_connection;
-	DBusConnection *     session_connection;
+	DBusConnection *     user_connection;
 	int                  ret;
+	char *               pidfile_path = NULL;
+	char *               pidfile = NULL;
+	char *               user_session_addr = NULL;
+	nih_local char **    user_session_path = NULL;
+	char *               path_element = NULL;
 
 
 	nih_main_init (argv[0]);
 
-	nih_option_set_synopsis (_("Bridge system upstart events into session upstart"));
+	nih_option_set_synopsis (_("Bridge system upstart events into the user session upstart"));
 	nih_option_set_help (
 		_("By default, upstart-event-bridge does not detach from the "
 		  "console and remains in the foreground.  Use the --daemon "
@@ -108,6 +113,12 @@
 	if (! args)
 		exit (1);
 
+	user_session_addr = getenv ("UPSTART_SESSION");
+	if (! user_session_addr) {
+		nih_fatal (_("UPSTART_SESSION isn't set in environment"));
+		exit (1);
+	}
+
 	/* Initialise the connection to system Upstart */
 	system_connection = NIH_SHOULD (nih_dbus_bus (DBUS_BUS_SYSTEM, upstart_disconnected));
 
@@ -160,24 +171,24 @@
 		exit (1);
 	}
 
-	/* Initialise the connection to session Upstart */
-	session_connection = nih_dbus_bus (DBUS_BUS_SESSION, upstart_disconnected);
+	/* Initialise the connection to user session Upstart */
+	user_connection = NIH_SHOULD (nih_dbus_connect (user_session_addr, upstart_disconnected));
 
-	if (! session_connection) {
+	if (! user_connection) {
 		NihError *err;
 
 		err = nih_error_get ();
-		nih_fatal ("%s: %s", _("Could not connect to session Upstart"),
+		nih_fatal ("%s: %s", _("Could not connect to the user session Upstart"),
 			   err->message);
 		nih_free (err);
 
 		exit (1);
 	}
 
-	session_upstart = NIH_SHOULD (nih_dbus_proxy_new (NULL, session_connection,
-						  DBUS_SERVICE_UPSTART, DBUS_PATH_UPSTART,
+	user_upstart = NIH_SHOULD (nih_dbus_proxy_new (NULL, user_connection,
+						  NULL, DBUS_PATH_UPSTART,
 						  NULL, NULL));
-	if (! session_upstart) {
+	if (! user_upstart) {
 		NihError *err;
 
 		err = nih_error_get ();
@@ -190,6 +201,32 @@
 
 	/* Become daemon */
 	if (daemonise) {
+		/* Deal with the pidfile location when becoming a daemon.
+		 * We need to be able to run one bridge per upstart daemon.
+		 * Store the PID file in XDG_RUNTIME_DIR or HOME and include the pid of
+		 * the Upstart instance (last part of the DBus path) in the filename.
+		 */
+
+		/* Extract PID from UPSTART_SESSION */
+		user_session_path = nih_str_split (NULL, user_session_addr, "/", TRUE);
+		for (int i = 0; user_session_path[i] != NULL; i++)
+			path_element = user_session_path[i];
+
+		if (! path_element) {
+			nih_fatal (_("Invalid value for UPSTART_SESSION"));
+			exit (1);
+		}
+
+		pidfile_path = getenv ("XDG_RUNTIME_DIR");
+		if (!pidfile_path)
+			pidfile_path = getenv ("HOME");
+
+		if (pidfile_path) {
+			NIH_MUST (nih_strcat_sprintf (&pidfile, NULL, "%s/upstart-event-bridge.%s.pid",
+					                        pidfile_path, path_element));
+			nih_main_set_pidfile (pidfile);
+		}
+
 		if (nih_main_daemonise () < 0) {
 			NihError *err;
 
@@ -213,6 +250,11 @@
 
 	ret = nih_main_loop ();
 
+	/* Destroy any PID file we may have created */
+	if (daemonise) {
+		nih_main_unlink_pidfile();
+	}
+
 	return ret;
 }
 
@@ -253,7 +295,7 @@
 	NIH_MUST (nih_strcat_sprintf (&new_event_name, NULL, ":sys:%s", event_name));
 
 	/* Re-transmit the event */
-	pending_call = upstart_emit_event (session_upstart,
+	pending_call = upstart_emit_event (user_upstart,
 			new_event_name, event_env, FALSE,
 			NULL, emit_event_error, NULL,
 			NIH_DBUS_TIMEOUT_NEVER);
@@ -277,7 +319,7 @@
 	DBusPendingCall *   pending_call;
 
 	/* Re-transmit the event */
-	pending_call = upstart_emit_event (session_upstart,
+	pending_call = upstart_emit_event (user_upstart,
 			":sys:restarted", NULL, FALSE,
 			NULL, emit_event_error, NULL,
 			NIH_DBUS_TIMEOUT_NEVER);