upstart

Merge lp:~stgraber/upstart/upstart-session-socket into lp:upstart

  1. upstart-session-socket
  2. Merge into trunk
Proposed by Stéphane Graber
Status: Merged
Merged at revision: 1427
Proposed branch: lp:~stgraber/upstart/upstart-session-socket
Merge into: lp:upstart
Diff against target: 417 lines (+189/-33)
8 files modified
init/control.c (+16/-1)
init/job_process.c (+6/-0)
init/main.c (+5/-25)
init/man/init.5 (+4/-0)
init/state.c (+0/-1)
util/initctl.c (+34/-5)
util/man/initctl.8 (+7/-0)
util/tests/test_initctl.c (+117/-1)
To merge this branch: bzr merge lp:~stgraber/upstart/upstart-session-socket
Reviewer Review Type Date Requested Status
James Hunt Approve
Review via email: mp+143344@code.launchpad.net

Description of the change

This branch implements the UPSTART_SESSION variable from the user session
specification.

Upstart will now always listen to a private DBus socket.
The system instance remains on its usual /com/ubuntu/upstart address.
The user instances use /com/ubuntu/upstart-session/%uid/%pid

The path is then exported as UPSTART_SESSION in the job environments, so
that jobs can call initctl and talk to upstart without requiring DBus.

A new --user flag is also added to initctl, when passed, it'll make initctl
use the socket defined in UPSTART_SESSION if present or otherwise will
fallback to the DBus session bus.

To post a comment you must log in.
Revision history for this message
James Hunt (jamesodhunt) wrote :

* conf/rc-sysinit.conf: I've got some reservations about changing the
  default jobs we provide. That said, those building systems should
  really be creating their own jobs. I think I'd be happy if we added a
  note to the README stating that the files in 'conf/' are examples only
  and *will* need to be changed by those creating systems using Upstart.
* conf/rc.conf: As above.
* init/control.c: control_init(): Spaces before '(' (4 occurences).
* init/job_process.c:
* init/main.c:
* util/initctl.c:
  - user_mode: Comment should be something like 'if TRUE, talk to
    Upstart over a private D-Bus socket'.
  - upstart_open():
    - Space before '(' of getenv call (2 occurences).
    - I'd be tempted to save the value of the first call to getenv to
      avoid the 2nd call.
    - 'else' should be on same line as closing brace of 'if'.
    - Shouldn't the logic be inverted here to conform to?:
      https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions#Effect_of_UPSTART_SESSION
      So, by default, if UPSTART_SESSION is set, use the private D-Bus
      socket, unless the user specifies '--system'.
  - options: I'd be tempted to change 'start' to 'run' as the former
    implies a long-running operation.
* init/man/init.5: 'Job environment' section needs to be updated for
  'UPSTART_SESSION'.
* util/man/initctl.8: Need to document '--user'.
* util/tests/test_initctl.c: We could do with a test for '--user'. This
  will necessitate something like:
  - Changing _START_UPSTART() into a function and allowing atleast 1 extra
    argument to be specified or specifying the type (meaning '--user' or
    '--session') would work, along with a test
  - Changing WAIT_FOR_UPSTART() into a function to allow the session
    bus or private socket to be connected to.
  - Then, a test to simply start upstart, wait for it, create a conf
    file, check that initctl list shows that job, then stop upstart
    (see test_list()).

lp:~stgraber/upstart/upstart-session-socket updated
1425. By Stéphane Graber

Also listen for private connections when running as a user, but on a different address.

1426. By Stéphane Graber

Export UPSTART_SESSION in the jobs environment

1427. By Stéphane Graber

Add user mode flag to initctl

1428. By Stéphane Graber

Set the right bus address for user mode

Revision history for this message
Stéphane Graber (stgraber) wrote :

> * conf/rc-sysinit.conf: I've got some reservations about changing the
> default jobs we provide. That said, those building systems should
> really be creating their own jobs. I think I'd be happy if we added a
> note to the README stating that the files in 'conf/' are examples only
> and *will* need to be changed by those creating systems using Upstart.
> * conf/rc.conf: As above.

Those two ended up in this branch by mistake, that should be fixed now.

> * init/control.c: control_init(): Spaces before '(' (4 occurences).
> * init/job_process.c:
> * init/main.c:
> * util/initctl.c:
> - user_mode: Comment should be something like 'if TRUE, talk to
> Upstart over a private D-Bus socket'.
> - upstart_open():
> - Space before '(' of getenv call (2 occurences).
> - I'd be tempted to save the value of the first call to getenv to
> avoid the 2nd call.
> - 'else' should be on same line as closing brace of 'if'.
> - Shouldn't the logic be inverted here to conform to?:
> https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions#
> Effect_of_UPSTART_SESSION
> So, by default, if UPSTART_SESSION is set, use the private D-Bus
> socket, unless the user specifies '--system'.
> - options: I'd be tempted to change 'start' to 'run' as the former
> implies a long-running operation.
> * init/man/init.5: 'Job environment' section needs to be updated for
> 'UPSTART_SESSION'.
> * util/man/initctl.8: Need to document '--user'.
> * util/tests/test_initctl.c: We could do with a test for '--user'. This
> will necessitate something like:
> - Changing _START_UPSTART() into a function and allowing atleast 1 extra
> argument to be specified or specifying the type (meaning '--user' or
> '--session') would work, along with a test
> - Changing WAIT_FOR_UPSTART() into a function to allow the session
> bus or private socket to be connected to.
> - Then, a test to simply start upstart, wait for it, create a conf
> file, check that initctl list shows that job, then stop upstart
> (see test_list()).

lp:~stgraber/upstart/upstart-session-socket updated
1429. By Stéphane Graber

Add missing spaces before '('.

1430. By Stéphane Graber

Add UPSTART_SESSION to manpage.

1431. By Stéphane Graber

Add --user to the manpage

1432. By Stéphane Graber

Update comment for user_mode.

1433. By Stéphane Graber

Avoid duplicate call to getenv

1434. By Stéphane Graber

Add missing spaces before '('.

1435. By Stéphane Graber

Syntax fix and change wording of the help message a bit.

Revision history for this message
Stéphane Graber (stgraber) wrote :

> > * conf/rc-sysinit.conf: I've got some reservations about changing the
> > default jobs we provide. That said, those building systems should
> > really be creating their own jobs. I think I'd be happy if we added a
> > note to the README stating that the files in 'conf/' are examples only
> > and *will* need to be changed by those creating systems using Upstart.
> > * conf/rc.conf: As above.
>
> Those two ended up in this branch by mistake, that should be fixed now.
>
> > * init/control.c: control_init(): Spaces before '(' (4 occurences).

Fixed.

> > * init/job_process.c:
> > * init/main.c:
> > * util/initctl.c:
> > - user_mode: Comment should be something like 'if TRUE, talk to
> > Upstart over a private D-Bus socket'.

Changed.

> > - upstart_open():
> > - Space before '(' of getenv call (2 occurences).

Just one after the fix below, but fixed :)

> > - I'd be tempted to save the value of the first call to getenv to
> > avoid the 2nd call.

Fixed.

> > - 'else' should be on same line as closing brace of 'if'.

Fixed. Working on 3 different C projects each with different coding guideline is a real source of headache ;)

> > - Shouldn't the logic be inverted here to conform to?:
> >
> https://wiki.ubuntu.com/FoundationsTeam/Specs/RaringUpstartUserSessions#
> > Effect_of_UPSTART_SESSION
> > So, by default, if UPSTART_SESSION is set, use the private D-Bus
> > socket, unless the user specifies '--system'.

Yeah, that part is still a bit blurry in my head, I'll try to catch you tomorrow to discuss this some more.

Does that table mean we won't ever look for upstart on the session bus when passed --user?

Also, I'm at a bit of a loss as to how "initctl --user" as root without UPSTART_SESSION isn't failing according to that table?

> > - options: I'd be tempted to change 'start' to 'run' as the former
> > implies a long-running operation.

Done, was a copy/paste from main.c.

> > * init/man/init.5: 'Job environment' section needs to be updated for
> > 'UPSTART_SESSION'.

Done

> > * util/man/initctl.8: Need to document '--user'.

Done

> > * util/tests/test_initctl.c: We could do with a test for '--user'. This
> > will necessitate something like:
> > - Changing _START_UPSTART() into a function and allowing atleast 1 extra
> > argument to be specified or specifying the type (meaning '--user' or
> > '--session') would work, along with a test
> > - Changing WAIT_FOR_UPSTART() into a function to allow the session
> > bus or private socket to be connected to.
> > - Then, a test to simply start upstart, wait for it, create a conf
> > file, check that initctl list shows that job, then stop upstart
> > (see test_list()).

Will look into this once we agree on what initctl should do :)

lp:~stgraber/upstart/upstart-session-socket updated
1436. By Stéphane Graber

Update code to match spec.

1437. By Stéphane Graber

Update upstart code to behave the same way as initctl. That's, don't bind the session bus unless passed --session.

Revision history for this message
Stéphane Graber (stgraber) wrote :

Alright, I think the current version of the branch matches the spec.

I also included a change to upstart itself, so that --user doesn't mean that it'll listen to the session bus, if you want to force it to listen to the session bus and do user session stuff, --session --user is what you want.

Revision history for this message
James Hunt (jamesodhunt) wrote :

Hi Stéphane,

* init/control.c:
  - control_init(): Second call to nih_strcat_sprintf() should be
    nih_strcat() or nih_strdup().
* util/tests/test_initctl.c: Still need a test for 'init --user'.

review: Needs Fixing
lp:~stgraber/upstart/upstart-session-socket updated
1438. By Stéphane Graber

Use nih_strdup instead of nih_strcat_sprintf

1439. By Stéphane Graber

Revert main.c hunk that was making upstart always listen for private connections. It's no longer required now that user_mode is separate from use_session_bus.

1440. By Stéphane Graber

Add check for user-mode failure on missing env variable.

1441. By Stéphane Graber

Add test for UPSTART_SESSION

1442. By Stéphane Graber

Update manpage

Revision history for this message
Stéphane Graber (stgraber) wrote :

I replaced the nih_strcat_sprintf by a nih_strdup.

I also dropped some changes to main.c that weren't relevant after the clear split between use_session_bus and user_session. That makes "/sbin/init --user --session" now possible.

And added two tests:
 - Check that initctl connects to the user bus when user_mode is set to TRUE and UPSTART_SESSION is set.
 - Check that initctl fails when user_mode is set to TRUE and UPSTART_SESSION isn't set.

I also updated the manpage to reflect the change in behaviour (now matching the spec).

lp:~stgraber/upstart/upstart-session-socket updated
1443. By Stéphane Graber

Code style

Revision history for this message
James Hunt (jamesodhunt) wrote :

Thanks! LGTM - merged.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'init/control.c'
2--- init/control.c 2012-12-17 11:45:28 +0000
3+++ init/control.c 2013-01-22 18:19:21 +0000
4@@ -76,11 +76,18 @@
5 int use_session_bus = FALSE;
6
7 /**
8+ * user_mode:
9+ *
10+ * If TRUE, upstart runs in user session mode.
11+ **/
12+int user_mode = FALSE;
13+
14+/**
15 * control_server_address:
16 *
17 * Address on which the control server may be reached.
18 **/
19-const char *control_server_address = DBUS_ADDRESS_UPSTART;
20+char *control_server_address = NULL;
21
22 /**
23 * control_server:
24@@ -116,6 +123,14 @@
25 {
26 if (! control_conns)
27 control_conns = NIH_MUST (nih_list_new (NULL));
28+
29+ if (! control_server_address) {
30+ if (user_mode)
31+ NIH_MUST (nih_strcat_sprintf (&control_server_address, NULL,
32+ "%s-session/%d/%d", DBUS_ADDRESS_UPSTART, getuid (), getpid ()));
33+ else
34+ control_server_address = nih_strdup (NULL, DBUS_ADDRESS_UPSTART);
35+ }
36 }
37
38
39
40=== modified file 'init/job_process.c'
41--- init/job_process.c 2012-12-18 09:02:24 +0000
42+++ init/job_process.c 2013-01-22 18:19:21 +0000
43@@ -65,6 +65,7 @@
44 #include "job_class.h"
45 #include "job.h"
46 #include "errors.h"
47+#include "control.h"
48
49
50 /**
51@@ -126,6 +127,8 @@
52 static void job_process_trace_fork (Job *job, ProcessType process);
53 static void job_process_trace_exec (Job *job, ProcessType process);
54
55+extern int user_mode;
56+extern char *control_server_address;
57
58 /**
59 * job_process_run:
60@@ -275,6 +278,9 @@
61 "UPSTART_JOB=%s", job->class->name));
62 NIH_MUST (environ_set (&env, NULL, &envc, TRUE,
63 "UPSTART_INSTANCE=%s", job->name));
64+ if (user_mode)
65+ NIH_MUST (environ_set (&env, NULL, &envc, TRUE,
66+ "UPSTART_SESSION=%s", control_server_address));
67
68 /* If we're about to spawn the main job and we expect it to become
69 * a daemon or fork before we can move out of spawned, we need to
70
71=== modified file 'init/main.c'
72--- init/main.c 2012-12-18 14:09:55 +0000
73+++ init/main.c 2013-01-22 18:19:21 +0000
74@@ -87,7 +87,6 @@
75
76 static void handle_confdir (void);
77 static void handle_logdir (void);
78-static void handle_usermode (void);
79 static int console_type_setter (NihOption *option, const char *arg);
80
81
82@@ -121,13 +120,7 @@
83 **/
84 static int disable_startup_event = FALSE;
85
86-/**
87- * user_mode:
88- *
89- * If TRUE, upstart runs in user session mode.
90- **/
91-static int user_mode = FALSE;
92-
93+extern int user_mode;
94 extern int disable_sessions;
95 extern int disable_job_logging;
96 extern int use_session_bus;
97@@ -207,7 +200,6 @@
98
99 handle_confdir ();
100 handle_logdir ();
101- handle_usermode ();
102
103 if (disable_job_logging)
104 nih_debug ("Job logging disabled");
105@@ -215,7 +207,7 @@
106 control_handle_bus_type ();
107
108 #ifndef DEBUG
109- if (use_session_bus == FALSE) {
110+ if (use_session_bus == FALSE && user_mode == FALSE) {
111
112 int needs_devtmpfs = 0;
113
114@@ -391,7 +383,7 @@
115 nih_signal_reset ();
116
117 #ifndef DEBUG
118- if (use_session_bus == FALSE) {
119+ if (use_session_bus == FALSE && user_mode == FALSE) {
120 /* Catch fatal errors immediately rather than waiting for a new
121 * iteration through the main loop.
122 */
123@@ -408,7 +400,7 @@
124 nih_signal_set_handler (SIGALRM, nih_signal_handler);
125
126 #ifndef DEBUG
127- if (use_session_bus == FALSE) {
128+ if (use_session_bus == FALSE && user_mode == FALSE) {
129 /* Ask the kernel to send us SIGINT when control-alt-delete is
130 * pressed; generate an event with the same name.
131 */
132@@ -581,7 +573,7 @@
133 }
134
135 #ifndef DEBUG
136- if (use_session_bus == FALSE) {
137+ if (use_session_bus == FALSE && user_mode == FALSE) {
138 /* Now that the startup is complete, send all further logging output
139 * to kmsg instead of to the console.
140 */
141@@ -970,18 +962,6 @@
142 log_dir);
143 }
144
145-/**
146- * handle_usermode:
147- *
148- * Setup user session mode.
149- **/
150-static void
151-handle_usermode (void)
152-{
153- if (user_mode)
154- use_session_bus = TRUE;
155-}
156-
157 /**
158 * NihOption setter function to handle selection of default console
159 * type.
160
161=== modified file 'init/man/init.5'
162--- init/man/init.5 2012-12-18 17:40:49 +0000
163+++ init/man/init.5 2013-01-22 18:19:21 +0000
164@@ -444,6 +444,10 @@
165 .BR initctl (8)
166 utility to default to acting on the job the commands are called from.
167
168+When running in a user session, the additional UPSTART_SESSION environment
169+variable is also set and contains the DBus path to the private bus used by
170+the upstart instance daemon.
171+
172 .TP
173 .B env \fIKEY\fR[=\fIVALUE\fR]
174 Defines a default environment variable, the value of which may be overridden
175
176=== modified file 'init/state.c'
177--- init/state.c 2012-12-07 18:26:43 +0000
178+++ init/state.c 2013-01-22 18:19:21 +0000
179@@ -48,7 +48,6 @@
180 json_object *json_events = NULL;
181 json_object *json_classes = NULL;
182
183-extern int use_session_bus;
184 extern char *log_dir;
185
186 /**
187
188=== modified file 'util/initctl.c'
189--- util/initctl.c 2012-03-16 21:02:13 +0000
190+++ util/initctl.c 2013-01-22 18:19:21 +0000
191@@ -145,6 +145,14 @@
192 int dbus_bus_type = -1;
193
194 /**
195+ * user_mode:
196+ *
197+ * If TRUE, talk to Upstart over the private socket defined in UPSTART_SESSION
198+ * if UPSTART_SESSION isn't defined, then fallback to the session bus.
199+ **/
200+int user_mode = FALSE;
201+
202+/**
203 * dest_name:
204 *
205 * Name on the D-Bus system bus that the message should be sent to when
206@@ -291,11 +299,30 @@
207 DBusError dbus_error;
208 DBusConnection *connection;
209 NihDBusProxy * upstart;
210-
211- if (use_dbus < 0)
212- use_dbus = getuid () ? TRUE : FALSE;
213- if (use_dbus >= 0 && dbus_bus_type < 0)
214- dbus_bus_type = DBUS_BUS_SYSTEM;
215+ char * user_addr;
216+
217+ user_addr = getenv ("UPSTART_SESSION");
218+
219+ if (user_addr && dbus_bus_type < 0) {
220+ user_mode = TRUE;
221+ }
222+
223+ if (! user_mode) {
224+ if (use_dbus < 0)
225+ use_dbus = getuid () ? TRUE : FALSE;
226+ if (use_dbus >= 0 && dbus_bus_type < 0)
227+ dbus_bus_type = DBUS_BUS_SYSTEM;
228+ }
229+ else {
230+ if (! user_addr) {
231+ nih_error ("UPSTART_SESSION isn't set in the environment. "
232+ "Unable to locate the Upstart instance.");
233+ return NULL;
234+ }
235+ dest_address = user_addr;
236+ use_dbus = FALSE;
237+ }
238+
239
240 dbus_error_init (&dbus_error);
241 if (use_dbus) {
242@@ -2356,6 +2383,8 @@
243 NULL, NULL, NULL, dbus_bus_type_setter },
244 { 0, "dest", N_("destination well-known name on D-Bus bus"),
245 NULL, "NAME", &dest_name, NULL },
246+ { 0, "user", N_("run in user mode (as used for user sessions)"),
247+ NULL, NULL, &user_mode, NULL },
248
249 NIH_OPTION_LAST
250 };
251
252=== modified file 'util/man/initctl.8'
253--- util/man/initctl.8 2012-08-31 21:01:48 +0000
254+++ util/man/initctl.8 2013-01-22 18:19:21 +0000
255@@ -39,6 +39,13 @@
256 .\"
257 .SH OPTIONS
258 .TP
259+.B \-\-user
260+User mode. In this mode, initctl will talk to the
261+.BR init (8)
262+daemon using the D\-Bus private socket defined in the UPSTART_SESSION
263+environment variable.
264+.\"
265+.TP
266 .B \-\-session
267 Connect to
268 .BR init (8)
269
270=== modified file 'util/tests/test_initctl.c'
271--- util/tests/test_initctl.c 2012-10-26 16:28:12 +0000
272+++ util/tests/test_initctl.c 2013-01-22 18:19:21 +0000
273@@ -29,7 +29,7 @@
274 #include <signal.h>
275 #include <unistd.h>
276 #include <regex.h>
277-#include <sys/types.h>
278+#include <sys/types.h>
279 #include <sys/stat.h>
280
281 #include <nih-dbus/dbus_error.h>
282@@ -379,6 +379,7 @@
283 }
284
285 extern int use_dbus;
286+extern int user_mode;
287 extern int dbus_bus_type;
288 extern char *dest_name;
289 extern const char *dest_address;
290@@ -508,6 +509,91 @@
291 dbus_shutdown ();
292 }
293
294+ /* Check that we can create a proxy to Upstart's private internal
295+ * server in user mode, and that this is the default behaviour if we don't
296+ * fiddle with the other options. The returned proxy should
297+ * hold the only reference to the connection.
298+ */
299+ TEST_FEATURE ("with user-mode");
300+ TEST_ALLOC_FAIL {
301+ use_dbus = -1;
302+ dbus_bus_type = -1;
303+ dest_name = NULL;
304+ dest_address = DBUS_ADDRESS_UPSTART;
305+ user_mode = TRUE;
306+
307+ assert0 (setenv ("UPSTART_SESSION",
308+ "unix:abstract=/com/ubuntu/upstart/test-session",
309+ TRUE));
310+
311+ TEST_ALLOC_SAFE {
312+ server = nih_dbus_server (getenv ("UPSTART_SESSION"),
313+ my_connect_handler,
314+ NULL);
315+ assert (server != NULL);
316+ }
317+
318+ my_connect_handler_called = FALSE;
319+ last_connection = NULL;
320+
321+ TEST_DIVERT_STDERR (output) {
322+ proxy = upstart_open (NULL);
323+ }
324+ rewind (output);
325+
326+ if (test_alloc_failed
327+ && (proxy == NULL)) {
328+ TEST_FILE_EQ (output, "test: Cannot allocate memory\n");
329+ TEST_FILE_END (output);
330+ TEST_FILE_RESET (output);
331+
332+ if (last_connection) {
333+ dbus_connection_close (last_connection);
334+ dbus_connection_unref (last_connection);
335+ }
336+
337+ dbus_server_disconnect (server);
338+ dbus_server_unref (server);
339+
340+ dbus_shutdown ();
341+ continue;
342+ }
343+
344+ nih_main_loop ();
345+
346+ TEST_TRUE (my_connect_handler_called);
347+ TEST_NE_P (last_connection, NULL);
348+
349+ TEST_NE_P (proxy, NULL);
350+ TEST_ALLOC_SIZE (proxy, sizeof (NihDBusProxy));
351+
352+ TEST_NE_P (proxy->connection, NULL);
353+ TEST_EQ_P (proxy->name, NULL);
354+ TEST_EQ_P (proxy->owner, NULL);
355+ TEST_EQ_STR (proxy->path, DBUS_PATH_UPSTART);
356+ TEST_ALLOC_PARENT (proxy->path, proxy);
357+ TEST_FALSE (proxy->auto_start);
358+
359+ TEST_EQ_P (proxy->lost_handler, NULL);
360+ TEST_EQ_P (proxy->data, NULL);
361+
362+ nih_free (proxy);
363+
364+ TEST_FILE_END (output);
365+ TEST_FILE_RESET (output);
366+
367+ dbus_connection_close (last_connection);
368+ dbus_connection_unref (last_connection);
369+
370+ dbus_server_disconnect (server);
371+ dbus_server_unref (server);
372+
373+ dbus_shutdown ();
374+
375+ unsetenv ("UPSTART_SESSION");
376+ user_mode = FALSE;
377+ }
378+
379
380 /* Check that we can create a connection to Upstart via the system
381 * bus. The returned proxy should use the default name on that
382@@ -719,6 +805,36 @@
383 }
384
385
386+ /* Check that when we attempt to connect to Upstart in user mode but
387+ * without UPSTART_SESSION set in the environment, an appropriate
388+ * error is output.
389+ */
390+ TEST_FEATURE ("with user-mode and no target");
391+ TEST_ALLOC_FAIL {
392+ use_dbus = -1;
393+ dbus_bus_type = -1;
394+ dest_name = NULL;
395+ dest_address = DBUS_ADDRESS_UPSTART;
396+ user_mode = TRUE;
397+
398+ unsetenv ("UPSTART_SESSION");
399+
400+ TEST_DIVERT_STDERR (output) {
401+ proxy = upstart_open (NULL);
402+ }
403+ rewind (output);
404+
405+ TEST_EQ_P (proxy, NULL);
406+
407+ TEST_FILE_EQ (output, ("test: UPSTART_SESSION isn't set in the environment. "
408+ "Unable to locate the Upstart instance.\n"));
409+ TEST_FILE_END (output);
410+ TEST_FILE_RESET (output);
411+
412+ dbus_shutdown ();
413+ user_mode = FALSE;
414+ }
415+
416 fclose (output);
417 }
418

Subscribers

People subscribed via source and target branches