Merge lp:~serge-hallyn/ubuntu/utopic/systemd-shim/shim-abandoncg into lp:ubuntu/utopic/systemd-shim
- Utopic (14.10)
- shim-abandoncg
- Merge into utopic
Status: | Work in progress |
---|---|
Proposed branch: | lp:~serge-hallyn/ubuntu/utopic/systemd-shim/shim-abandoncg |
Merge into: | lp:ubuntu/utopic/systemd-shim |
Diff against target: |
759 lines (+740/-0) 3 files modified
debian/changelog (+8/-0) debian/patches/0001-Implement-Manager-StopUnit-and-Scope-Abandon.patch (+731/-0) debian/patches/series (+1/-0) |
To merge this branch: | bzr merge lp:~serge-hallyn/ubuntu/utopic/systemd-shim/shim-abandoncg |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ubuntu branches | Pending | ||
Review via email: mp+233280@code.launchpad.net |
Commit message
Description of the change
This is the proposed fix for 1359439.
Note that if it is possible for upstart job pre-start and start scripts to have lingering tasks, and if KillUserProcess
James Hunt (jamesodhunt) wrote : | # |
I have tested this code in combination with the Upstart fix for bug 1357252 and cannot now make a cgroup upstart job fail to start.
Serge Hallyn (serge-hallyn) wrote : | # |
Quoting James Hunt (<email address hidden>):
> This fix is also required for bug 1357252.
>
> The code changes look fine to me (although I'm not very familiar with the codebase). Just a few queries:
>
> - src/systemd-shim.c : There's a chunk of code that's #if-0'd out. Can this be removed?
It can, but I'd rather not. It's a reminder that we have got to either
find a way to make it short-lived again, or address any issues which
were ignored previously due to systemd-shim being short-lived. This
includes closing or at least re-opening on peer-closure of the cgmanager
socket and running systemd-shim under valgrind to check for memleaks.
> - I was imagining when I heard that systemd-shim was now long-running that it would exit when the session exits but I now see that it starts on-demand and then stays running for the duration of the boot to allow it to service requests from multiple sessions. Now that it is maintaining state, what's the implication of it stopping on error and being restarted?
StopUnit will continue to work, Abandon will be ignored. So on a default
Ubuntu desktop, cgroups for any active sessions will not be deleted.
> - Does the shim need an apparmor policy now it's long-running?
D'oh, that wouldn't hurt. shim really shouldn't need much access at all.
Unmerged revisions
- 18. By Serge Hallyn
-
fix signed/unsigned in sscanf
- 17. By Serge Hallyn
-
Implement StopUnit and Abandon to remove cgroups at logout, and stop
setting remove-on-empty flag on cgroups at login.
(LP: #1355966)
Preview Diff
1 | === removed file '.pc/applied-patches' |
2 | === modified file 'debian/changelog' |
3 | --- debian/changelog 2014-08-20 08:22:08 +0000 |
4 | +++ debian/changelog 2014-09-03 22:02:47 +0000 |
5 | @@ -1,3 +1,11 @@ |
6 | +systemd-shim (7-1ubuntu1) utopic; urgency=medium |
7 | + |
8 | + * Implement StopUnit and Abandon to remove cgroups at logout, and stop |
9 | + setting remove-on-empty flag on cgroups at login. |
10 | + (LP: #1355966) |
11 | + |
12 | + -- Serge Hallyn <serge.hallyn@ubuntu.com> Wed, 03 Sep 2014 16:42:07 -0500 |
13 | + |
14 | systemd-shim (7-1) unstable; urgency=medium |
15 | |
16 | * New upstream release: |
17 | |
18 | === added directory 'debian/patches' |
19 | === added file 'debian/patches/0001-Implement-Manager-StopUnit-and-Scope-Abandon.patch' |
20 | --- debian/patches/0001-Implement-Manager-StopUnit-and-Scope-Abandon.patch 1970-01-01 00:00:00 +0000 |
21 | +++ debian/patches/0001-Implement-Manager-StopUnit-and-Scope-Abandon.patch 2014-09-03 22:02:47 +0000 |
22 | @@ -0,0 +1,731 @@ |
23 | +From 0a3ebe04689c7bbcfcd312f697aee08a54c66aef Mon Sep 17 00:00:00 2001 |
24 | +From: Serge Hallyn <serge.hallyn@ubuntu.com> |
25 | +Date: Tue, 2 Sep 2014 11:03:21 -0500 |
26 | +Subject: [PATCH 1/1] Implement Manager:StopUnit and Scope:Abandon |
27 | + |
28 | +First, stop setting removeonempty at login. This is in particular |
29 | +causing problems for upstart. Upstart uses the same cgroup name |
30 | +for several possible job states. Using removeonempty means that |
31 | +we have an unpredictable time at which the cgroup is removed. So |
32 | +when pre-start is over, post-stop may kick in, create the cgroup |
33 | +(which exists), now autoremove kicks in, then post-stop fails to |
34 | +move its child task into the cgroup. |
35 | + |
36 | +StopUnit is called when /etc/systemd/logind.conf has |
37 | + KillUserProcesses=yes |
38 | +It receives the scope (unique system-wide) to be stopped, and |
39 | +kills that scope. That means killing any tasks in the cgroup |
40 | +or its subdirectories, and removing the directories themselves. |
41 | +We first set removeonempty, then recursively kill subdirectories, |
42 | +then killall tasks, then try to remove the directory. Some kernels |
43 | +have a race which can cause a child cgroup to not yet be listed as |
44 | +removed by the time we get to removing the parent cgroup. That |
45 | +should not matter for us since we've set removeonempty, and the |
46 | +cgroup will get removed eventually anyway. However, in the case |
47 | +where the cgroup was already empty, removeonempty would do nothing |
48 | +until the next time the cgroup *becomes* empty, so we still need |
49 | +to try to remove the directory by hand. One of the other should |
50 | +always succeed. |
51 | + |
52 | +Abandon is sent differently than StopUnit - it is sent over a |
53 | +session-specific path, to interface org.freedesktop.systemd1.Scope. |
54 | +So shim needs to register the session-specific object path when it |
55 | +starts. This means that shim cannot exit until the session has |
56 | +stopped. |
57 | + |
58 | +If there is a way to tell the dbus system bus to start us back |
59 | +up when that path is accessed (which does not persist across reboot) |
60 | +then we could do that. Or, we can tweak the exit-on-inactivity |
61 | +code to extend timeout so long as there are open sessions. For now |
62 | +I simply commented it out. |
63 | + |
64 | +Something should definately be done, as the shim code has been written |
65 | +until now assuming it would not persist. For instance the cgmanager |
66 | +socket is never closed. If there isn't a way to avoid keeping the |
67 | +shim long-running, then we will need to address these matters. |
68 | + |
69 | +Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> |
70 | +--- |
71 | + data/org.freedesktop.systemd1.conf | 4 + |
72 | + src/cgmanager.c | 214 +++++++++++++++++++++++++++++++++++-- |
73 | + src/cgmanager.h | 2 + |
74 | + src/cgroup-unit.c | 17 ++- |
75 | + src/systemd-iface.h | 7 ++ |
76 | + src/systemd-shim.c | 193 ++++++++++++++++++++++++++++++--- |
77 | + src/unit.c | 33 ++++++ |
78 | + src/unit.h | 3 + |
79 | + 8 files changed, 448 insertions(+), 25 deletions(-) |
80 | + |
81 | +Index: systemd-shim/data/org.freedesktop.systemd1.conf |
82 | +=================================================================== |
83 | +--- systemd-shim.orig/data/org.freedesktop.systemd1.conf |
84 | ++++ systemd-shim/data/org.freedesktop.systemd1.conf |
85 | +@@ -86,6 +86,10 @@ |
86 | + send_interface="org.freedesktop.systemd1.Manager" |
87 | + send_member="Dump"/> |
88 | + |
89 | ++ <allow send_destination="org.freedesktop.systemd1" |
90 | ++ send_interface="org.freedesktop.systemd1.Scope" |
91 | ++ send_member="Abandon"/> |
92 | ++ |
93 | + <allow receive_sender="org.freedesktop.systemd1"/> |
94 | + </policy> |
95 | + |
96 | +Index: systemd-shim/src/cgmanager.c |
97 | +=================================================================== |
98 | +--- systemd-shim.orig/src/cgmanager.c |
99 | ++++ systemd-shim/src/cgmanager.c |
100 | +@@ -24,6 +24,7 @@ |
101 | + |
102 | + #include "cgmanager.h" |
103 | + |
104 | ++#include <stdbool.h> |
105 | + #include <gio/gio.h> |
106 | + |
107 | + #define CGM_DBUS_ADDRESS "unix:path=/sys/fs/cgroup/cgmanager/sock" |
108 | +@@ -84,18 +85,16 @@ log_warning_on_error (GObject *sour |
109 | + g_variant_unref (reply); |
110 | + else |
111 | + { |
112 | +- g_warning ("cgmanager method call failed: %s. Use G_DBUS_DEBUG=message for more info.", error->message); |
113 | ++ g_info ("cgmanager method call failed: %s. Use G_DBUS_DEBUG=message for more info.", error->message); |
114 | + g_error_free (error); |
115 | + } |
116 | + } |
117 | + |
118 | +-static void |
119 | +-cgmanager_call (const gchar *method_name, |
120 | +- GVariant *parameters, |
121 | +- const GVariantType *reply_type) |
122 | ++static GDBusConnection *connection; |
123 | ++static gboolean initialised; |
124 | ++ |
125 | ++static void cgmanager_connect_wrapper(void) |
126 | + { |
127 | +- static GDBusConnection *connection; |
128 | +- static gboolean initialised; |
129 | + |
130 | + /* Use a separate bool to prevent repeated attempts to connect to a |
131 | + * defunct cgmanager... |
132 | +@@ -114,6 +113,14 @@ cgmanager_call (const gchar *meth |
133 | + |
134 | + initialised = TRUE; |
135 | + } |
136 | ++} |
137 | ++ |
138 | ++static void |
139 | ++cgmanager_call (const gchar *method_name, |
140 | ++ GVariant *parameters, |
141 | ++ const GVariantType *reply_type) |
142 | ++{ |
143 | ++ cgmanager_connect_wrapper(); |
144 | + |
145 | + if (!connection) |
146 | + return; |
147 | +@@ -146,8 +153,6 @@ cgmanager_create (const gchar *path, |
148 | + |
149 | + for (i = 0; i < n_pids; i++) |
150 | + cgmanager_call ("MovePid", g_variant_new ("(ssi)", "all", path, pids[i]), G_VARIANT_TYPE_UNIT); |
151 | +- |
152 | +- cgmanager_call ("RemoveOnEmpty", g_variant_new ("(ss)", "all", path), G_VARIANT_TYPE_UNIT); |
153 | + } |
154 | + |
155 | + void |
156 | +@@ -164,3 +169,194 @@ cgmanager_moveself (void) |
157 | + { |
158 | + cgmanager_call ("MovePidAbs", g_variant_new ("(ssi)", "all", "/", getpid()), G_VARIANT_TYPE_UNIT); |
159 | + } |
160 | ++ |
161 | ++static bool cg_exists(const gchar *path) |
162 | ++{ |
163 | ++ cgmanager_connect_wrapper(); |
164 | ++ GVariant *reply; |
165 | ++ GError *error = NULL; |
166 | ++ |
167 | ++ if (!connection) |
168 | ++ return false; |
169 | ++ |
170 | ++ reply = g_dbus_connection_call_sync (connection, NULL, "/org/linuxcontainers/cgmanager", |
171 | ++ "org.linuxcontainers.cgmanager0_0", "GetTasks", |
172 | ++ g_variant_new("(ss)", "name=systemd", path), |
173 | ++ G_VARIANT_TYPE("(ai)"), G_DBUS_CALL_FLAGS_NONE, |
174 | ++ -1, NULL, &error); |
175 | ++ if (!reply) { |
176 | ++ // we expect error if path does not exist |
177 | ++ g_error_free (error); |
178 | ++ return false; |
179 | ++ } |
180 | ++ return true; |
181 | ++} |
182 | ++ |
183 | ++static gchar *get_cgpath_from_scope(const gchar *scope) |
184 | ++{ |
185 | ++ GVariant *reply, *array; |
186 | ++ GError *error = NULL; |
187 | ++ const gchar *child; |
188 | ++ int i, last; |
189 | ++ |
190 | ++ cgmanager_connect_wrapper(); |
191 | ++ if (!connection) |
192 | ++ return NULL; |
193 | ++ |
194 | ++ reply = g_dbus_connection_call_sync (connection, NULL, "/org/linuxcontainers/cgmanager", |
195 | ++ "org.linuxcontainers.cgmanager0_0", "ListChildren", |
196 | ++ g_variant_new("(ss)", "name=systemd", "user.slice"), |
197 | ++ G_VARIANT_TYPE("(as)"), G_DBUS_CALL_FLAGS_NONE, -1, NULL, &error); |
198 | ++ |
199 | ++ if (!reply) { |
200 | ++ g_warning ("Error getting list of sessions from cgmanager: %s", error->message); |
201 | ++ g_error_free (error); |
202 | ++ return NULL; |
203 | ++ } |
204 | ++ |
205 | ++ if (g_variant_n_children(reply) < 1) { |
206 | ++ g_variant_unref(reply); |
207 | ++ return NULL; |
208 | ++ } |
209 | ++ |
210 | ++ array = g_variant_get_child_value(reply, 0); |
211 | ++ if (!array) { |
212 | ++ g_variant_unref(reply); |
213 | ++ return NULL; |
214 | ++ } |
215 | ++ |
216 | ++ last = (int)g_variant_n_children(array); |
217 | ++ |
218 | ++ for (i = 0; i < last; i++) { |
219 | ++ g_variant_get_child (array, (gsize)i, "s", &child); |
220 | ++ gchar *path = g_strdup_printf("user.slice/%s/%s", child, scope); |
221 | ++ if (!path) { |
222 | ++ g_variant_unref(reply); |
223 | ++ g_variant_unref(array); |
224 | ++ return NULL; |
225 | ++ } |
226 | ++ if (cg_exists(path)) { |
227 | ++ g_variant_unref(array); |
228 | ++ g_variant_unref(reply); |
229 | ++ return path; |
230 | ++ } |
231 | ++ } |
232 | ++ g_variant_unref(array); |
233 | ++ g_variant_unref(reply); |
234 | ++ return NULL; |
235 | ++} |
236 | ++ |
237 | ++static void cg_rm_recursive(const gchar *path, bool killtasks) |
238 | ++{ |
239 | ++ GVariant *reply, *array; |
240 | ++ GError *error = NULL; |
241 | ++ const gchar *child; |
242 | ++ int i, last; |
243 | ++ |
244 | ++ cgmanager_connect_wrapper(); |
245 | ++ if (!connection) |
246 | ++ return; |
247 | ++ |
248 | ++ cgmanager_call ("RemoveOnEmpty", g_variant_new ("(ss)", "all", path), G_VARIANT_TYPE_UNIT); |
249 | ++ |
250 | ++ /* first kill any subdirs recursively */ |
251 | ++ reply = g_dbus_connection_call_sync (connection, NULL, "/org/linuxcontainers/cgmanager", |
252 | ++ "org.linuxcontainers.cgmanager0_0", "ListChildren", |
253 | ++ g_variant_new("(ss)", "name=systemd", path), |
254 | ++ G_VARIANT_TYPE("(as)"), G_DBUS_CALL_FLAGS_NONE, -1, NULL, &error); |
255 | ++ |
256 | ++ if (!reply) { |
257 | ++ g_warning ("Error getting list of sessions from cgmanager: %s", error->message); |
258 | ++ g_error_free (error); |
259 | ++ return; |
260 | ++ } |
261 | ++ |
262 | ++ if (g_variant_n_children(reply) < 1) { |
263 | ++ g_variant_unref(reply); |
264 | ++ return; |
265 | ++ } |
266 | ++ |
267 | ++ array = g_variant_get_child_value(reply, 0); |
268 | ++ if (!array) { |
269 | ++ g_variant_unref(reply); |
270 | ++ return; |
271 | ++ } |
272 | ++ |
273 | ++ last = (int)g_variant_n_children(array); |
274 | ++ |
275 | ++ for (i = 0; i < last; i++) { |
276 | ++ g_variant_get_child (array, (gsize)i, "s", &child); |
277 | ++ gchar *rpath = g_strdup_printf("%s/%s", path, child); |
278 | ++ if (!rpath) |
279 | ++ continue; |
280 | ++ cg_rm_recursive(rpath, killtasks); |
281 | ++ } |
282 | ++ g_variant_unref(array); |
283 | ++ g_variant_unref(reply); |
284 | ++ |
285 | ++ if (killtasks) { |
286 | ++ // kill any tasks here |
287 | ++ reply = g_dbus_connection_call_sync (connection, NULL, "/org/linuxcontainers/cgmanager", |
288 | ++ "org.linuxcontainers.cgmanager0_0", "GetTasks", |
289 | ++ g_variant_new("(ss)", "name=systemd", path), |
290 | ++ G_VARIANT_TYPE("(ai)"), G_DBUS_CALL_FLAGS_NONE, -1, NULL, &error); |
291 | ++ |
292 | ++ if (!reply) { |
293 | ++ g_warning ("Error getting list of sessions from cgmanager: %s", error->message); |
294 | ++ g_error_free (error); |
295 | ++ return; |
296 | ++ } |
297 | ++ |
298 | ++ if (g_variant_n_children(reply) < 1) { |
299 | ++ g_variant_unref(reply); |
300 | ++ return; |
301 | ++ } |
302 | ++ |
303 | ++ array = g_variant_get_child_value(reply, 0); |
304 | ++ if (!array) { |
305 | ++ g_variant_unref(reply); |
306 | ++ return; |
307 | ++ } |
308 | ++ |
309 | ++ last = (int)g_variant_n_children(array); |
310 | ++ |
311 | ++ for (i = 0; i < last; i++) { |
312 | ++ guint32 pid; |
313 | ++ g_variant_get_child (array, (gsize)i, "i", &pid); |
314 | ++ // XXX todo - should we be nicer here, do a sigterm and wait a bit? |
315 | ++ kill(pid, SIGKILL); |
316 | ++ } |
317 | ++ g_variant_unref(array); |
318 | ++ g_variant_unref(reply); |
319 | ++ } |
320 | ++ |
321 | ++ // and remove the directory |
322 | ++ cgmanager_remove(path); |
323 | ++ return; |
324 | ++} |
325 | ++ |
326 | ++void cgmanager_kill (const gchar *scope) |
327 | ++{ |
328 | ++ gchar *cgpath; |
329 | ++ |
330 | ++ cgpath = get_cgpath_from_scope(scope); |
331 | ++ if (!cgpath) |
332 | ++ return; |
333 | ++ |
334 | ++ cg_rm_recursive (cgpath, true); |
335 | ++ g_free(cgpath); |
336 | ++} |
337 | ++ |
338 | ++// Abandon means we don't want to kill the tasks, but we |
339 | ++// do want to remove the cgroup if/when it is empty. |
340 | ++void cgmanager_abandon (const gchar *scope) |
341 | ++{ |
342 | ++ gchar *cgpath; |
343 | ++ |
344 | ++ cgpath = get_cgpath_from_scope(scope); |
345 | ++ if (!cgpath) |
346 | ++ return; |
347 | ++ |
348 | ++ cg_rm_recursive (cgpath, false); |
349 | ++ g_free(cgpath); |
350 | ++} |
351 | +Index: systemd-shim/src/cgmanager.h |
352 | +=================================================================== |
353 | +--- systemd-shim.orig/src/cgmanager.h |
354 | ++++ systemd-shim/src/cgmanager.h |
355 | +@@ -34,4 +34,6 @@ void cgmanager_remove (const gchar *path |
356 | + |
357 | + void cgmanager_moveself (void); |
358 | + |
359 | ++void cgmanager_kill (const gchar *scope); |
360 | ++ |
361 | + #endif /* _cgmanager_h_ */ |
362 | +Index: systemd-shim/src/cgroup-unit.c |
363 | +=================================================================== |
364 | +--- systemd-shim.orig/src/cgroup-unit.c |
365 | ++++ systemd-shim/src/cgroup-unit.c |
366 | +@@ -148,10 +148,24 @@ cgroup_unit_start (Unit *unit) |
367 | + g_free (path); |
368 | + } |
369 | + |
370 | ++extern void remove_scopeinfo(const gchar *scope); |
371 | ++ |
372 | + static void |
373 | + cgroup_unit_stop (Unit *unit) |
374 | + { |
375 | +- /* for now, no-op */ |
376 | ++ CGroupUnit *cg = (CGroupUnit *)unit; |
377 | ++ |
378 | ++ cgmanager_kill(cg->name); |
379 | ++ remove_scopeinfo(cg->name); |
380 | ++} |
381 | ++ |
382 | ++static void |
383 | ++cgroup_unit_abandon (Unit *unit) |
384 | ++{ |
385 | ++ CGroupUnit *cg = (CGroupUnit *)unit; |
386 | ++ |
387 | ++ cgmanager_abandon(cg->name); |
388 | ++ remove_scopeinfo(cg->name); |
389 | + } |
390 | + |
391 | + static const gchar * |
392 | +@@ -183,5 +197,6 @@ cgroup_unit_class_init (UnitClass *class |
393 | + class->start_transient = cgroup_unit_start_transient; |
394 | + class->start = cgroup_unit_start; |
395 | + class->stop = cgroup_unit_stop; |
396 | ++ class->abandon = cgroup_unit_abandon; |
397 | + class->get_state = cgroup_unit_get_state; |
398 | + } |
399 | +Index: systemd-shim/src/systemd-iface.h |
400 | +=================================================================== |
401 | +--- systemd-shim.orig/src/systemd-iface.h |
402 | ++++ systemd-shim/src/systemd-iface.h |
403 | +@@ -43,4 +43,11 @@ const gchar *systemd_iface = |
404 | + "</interface>" |
405 | + "</node>"; |
406 | + |
407 | ++const gchar *scope_iface = |
408 | ++ "<node>" |
409 | ++ "<interface name='org.freedesktop.systemd1.Scope'>" |
410 | ++ "<method name='Abandon'/>" |
411 | ++ " </interface>" |
412 | ++ "</node>"; |
413 | ++ |
414 | + #endif |
415 | +Index: systemd-shim/src/systemd-shim.c |
416 | +=================================================================== |
417 | +--- systemd-shim.orig/src/systemd-shim.c |
418 | ++++ systemd-shim/src/systemd-shim.c |
419 | +@@ -26,11 +26,37 @@ |
420 | + |
421 | + #include <stdlib.h> |
422 | + |
423 | ++static int open_sessions, alloced_sessions; |
424 | ++ |
425 | ++GDBusInterfaceInfo *manager_info = NULL; |
426 | ++ |
427 | ++GDBusInterfaceVTable mngr_vtable; |
428 | ++ |
429 | ++struct session_bus { |
430 | ++ GDBusConnection *connection; |
431 | ++ guint rid; |
432 | ++ gchar *scope; |
433 | ++}; |
434 | ++struct session_bus *sessions; |
435 | ++ |
436 | ++#if 0 |
437 | ++/* |
438 | ++ * we need to keep the shim up as long as a session is open. We can |
439 | ++ * reintroduce the exit-on-inactivity by looking at open_sessions, or |
440 | ++ * if we can register the per-session paths with the system bus |
441 | ++ */ |
442 | ++static void had_activity (void); |
443 | ++ |
444 | + static gboolean |
445 | + exit_on_inactivity (gpointer user_data) |
446 | + { |
447 | + extern gboolean in_shutdown; |
448 | + |
449 | ++ if (open_sessions) { |
450 | ++ had_activity(); |
451 | ++ return FALSE; |
452 | ++ } |
453 | ++ |
454 | + if (!in_shutdown) |
455 | + { |
456 | + GDBusConnection *system_bus; |
457 | +@@ -55,9 +81,60 @@ had_activity (void) |
458 | + |
459 | + inactivity_timeout = g_timeout_add (10000, exit_on_inactivity, NULL); |
460 | + } |
461 | ++#else |
462 | ++static inline void had_activity(void) { } |
463 | ++#endif |
464 | ++ |
465 | ++static void assign_new_scopeinfo(GDBusConnection *connection, const gchar *scope) |
466 | ++{ |
467 | ++ GDBusNodeInfo *node; |
468 | ++ GDBusInterfaceInfo *iface; |
469 | ++ guint sessid; |
470 | ++ gchar path[1024]; |
471 | ++ |
472 | ++ if (sscanf(scope, "session-%u", &sessid) != 1) { |
473 | ++ g_info("Bad session scope: %s\n", scope); |
474 | ++ return; |
475 | ++ } |
476 | ++ node = g_dbus_node_info_new_for_xml (scope_iface, NULL); |
477 | ++ g_assert(node); |
478 | ++ iface = g_dbus_node_info_lookup_interface (node, "org.freedesktop.systemd1.Scope"); |
479 | ++ g_assert(iface); |
480 | ++ if (open_sessions >= alloced_sessions) { |
481 | ++ sessions = realloc(sessions, (alloced_sessions+5) * sizeof(struct session_bus)); |
482 | ++ g_assert(sessions); |
483 | ++ alloced_sessions += 5; |
484 | ++ } |
485 | ++ snprintf(path, 1024, "/org/freedesktop/systemd1/unit/session_2d%u_2escope", sessid); |
486 | ++ sessions[open_sessions].connection = connection; |
487 | ++ sessions[open_sessions].rid = g_dbus_connection_register_object( |
488 | ++ connection, path, iface, &mngr_vtable, NULL, NULL, NULL); |
489 | ++ if (!sessions[open_sessions].rid) { |
490 | ++ g_critical("Error registering object %s\n", path); |
491 | ++ exit(1); |
492 | ++ } |
493 | ++ sessions[open_sessions].scope = g_strdup(scope); |
494 | ++ g_assert(sessions[open_sessions].scope); |
495 | ++ open_sessions++; |
496 | ++} |
497 | ++ |
498 | ++void remove_scopeinfo(const gchar *scope) |
499 | ++{ |
500 | ++ int i; |
501 | ++ for (i = 0; i < open_sessions; i++) { |
502 | ++ if (strcmp(scope, sessions[i].scope) == 0) { |
503 | ++ int j; |
504 | ++ g_dbus_connection_unregister_object(sessions[j].connection, sessions[j].rid); |
505 | ++ for (j=i; j<open_sessions-1; j++) |
506 | ++ sessions[j] = sessions[j+1]; |
507 | ++ open_sessions--; |
508 | ++ return; |
509 | ++ } |
510 | ++ } |
511 | ++} |
512 | + |
513 | + static void |
514 | +-shim_method_call (GDBusConnection *connection, |
515 | ++mngr_method_call (GDBusConnection *connection, |
516 | + const gchar *sender, |
517 | + const gchar *object_path, |
518 | + const gchar *interface_name, |
519 | +@@ -162,23 +239,41 @@ shim_method_call (GDBusConnection |
520 | + "org.freedesktop.systemd1.Manager", "JobRemoved", |
521 | + g_variant_new ("(uoss)", 0, "/", unit_get_state(unit), "done"), NULL); |
522 | + g_variant_unref (properties); |
523 | ++ |
524 | ++ assign_new_scopeinfo(connection, unit_get_state(unit)); |
525 | + g_object_unref (unit); |
526 | + goto success; |
527 | + } |
528 | + } |
529 | ++ else if (g_str_equal (method_name, "Abandon")) |
530 | ++ { |
531 | ++ Unit *unit; |
532 | ++ |
533 | ++ unit = fake_unit (object_path); |
534 | ++ |
535 | ++ if (unit) |
536 | ++ { |
537 | ++ unit_abandon (unit); |
538 | ++ g_dbus_method_invocation_return_value (invocation, NULL); |
539 | ++ g_object_unref (unit); |
540 | ++ goto success; |
541 | ++ } |
542 | ++ } |
543 | + |
544 | + else |
545 | + g_assert_not_reached (); |
546 | + |
547 | +- g_dbus_method_invocation_return_gerror (invocation, error); |
548 | +- g_error_free (error); |
549 | ++ if (error) { |
550 | ++ g_dbus_method_invocation_return_gerror (invocation, error); |
551 | ++ g_error_free (error); |
552 | ++ } |
553 | + |
554 | + success: |
555 | + had_activity (); |
556 | + } |
557 | + |
558 | + static GVariant * |
559 | +-shim_get_property (GDBusConnection *connection, |
560 | ++mngr_get_property (GDBusConnection *connection, |
561 | + const gchar *sender, |
562 | + const gchar *object_path, |
563 | + const gchar *interface_name, |
564 | +@@ -197,22 +292,81 @@ shim_get_property (GDBusConnection *con |
565 | + return g_variant_new ("s", id); |
566 | + } |
567 | + |
568 | ++GDBusInterfaceVTable mngr_vtable = { |
569 | ++ mngr_method_call, |
570 | ++ mngr_get_property, |
571 | ++}; |
572 | ++ |
573 | ++static gchar ** |
574 | ++subtree_enumerate (GDBusConnection *connection, |
575 | ++ const gchar *sender, |
576 | ++ const gchar *object_path, |
577 | ++ gpointer user_data) |
578 | ++{ |
579 | ++ gchar **nodes; |
580 | ++ GPtrArray *p; |
581 | ++ |
582 | ++ p = g_ptr_array_new (); |
583 | ++ g_ptr_array_add (p, g_strdup ("Manager")); |
584 | ++ g_ptr_array_add (p, g_strdup ("Scope")); |
585 | ++ g_ptr_array_add (p, NULL); |
586 | ++ nodes = (gchar **) g_ptr_array_free (p, FALSE); |
587 | ++ |
588 | ++ return nodes; |
589 | ++} |
590 | ++ |
591 | ++static GDBusInterfaceInfo ** |
592 | ++subtree_introspect (GDBusConnection *connection, |
593 | ++ const gchar *sender, |
594 | ++ const gchar *object_path, |
595 | ++ const gchar *node, |
596 | ++ gpointer user_data) |
597 | ++{ |
598 | ++ GPtrArray *p; |
599 | ++ int i; |
600 | ++ |
601 | ++ p = g_ptr_array_new (); |
602 | ++ |
603 | ++ g_ptr_array_add (p, g_dbus_interface_info_ref (manager_info)); |
604 | ++ g_ptr_array_add (p, NULL); |
605 | ++ |
606 | ++ return (GDBusInterfaceInfo **) g_ptr_array_free (p, FALSE); |
607 | ++} |
608 | ++ |
609 | ++static const GDBusInterfaceVTable * |
610 | ++subtree_dispatch (GDBusConnection *connection, |
611 | ++ const gchar *sender, |
612 | ++ const gchar *object_path, |
613 | ++ const gchar *interface_name, |
614 | ++ const gchar *node, |
615 | ++ gpointer *out_user_data, |
616 | ++ gpointer user_data) |
617 | ++{ |
618 | ++ return &mngr_vtable; |
619 | ++} |
620 | ++ |
621 | ++const GDBusSubtreeVTable subtree_vtable = |
622 | ++{ |
623 | ++ subtree_enumerate, |
624 | ++ subtree_introspect, |
625 | ++ subtree_dispatch |
626 | ++}; |
627 | ++ |
628 | + static void |
629 | + shim_bus_acquired (GDBusConnection *connection, |
630 | + const gchar *name, |
631 | + gpointer user_data) |
632 | + { |
633 | +- GDBusInterfaceVTable vtable = { |
634 | +- shim_method_call, |
635 | +- shim_get_property, |
636 | +- }; |
637 | +- GDBusInterfaceInfo *iface; |
638 | +- GDBusNodeInfo *node; |
639 | ++ guint rid; |
640 | + |
641 | +- node = g_dbus_node_info_new_for_xml (systemd_iface, NULL); |
642 | +- iface = g_dbus_node_info_lookup_interface (node, "org.freedesktop.systemd1.Manager"); |
643 | +- g_dbus_connection_register_object (connection, "/org/freedesktop/systemd1", iface, &vtable, NULL, NULL, NULL); |
644 | +- g_dbus_node_info_unref (node); |
645 | ++ rid = g_dbus_connection_register_subtree (connection, |
646 | ++ "/org/freedesktop/systemd1", |
647 | ++ &subtree_vtable, |
648 | ++ G_DBUS_SUBTREE_FLAGS_NONE, |
649 | ++ NULL, /* user_data */ |
650 | ++ NULL, /* user_data_free_func */ |
651 | ++ NULL); /* GError** */ |
652 | ++ g_assert (rid > 0); |
653 | + } |
654 | + |
655 | + static void |
656 | +@@ -227,6 +381,13 @@ shim_name_lost (GDBusConnection *connect |
657 | + int |
658 | + main (void) |
659 | + { |
660 | ++ GDBusNodeInfo *node; |
661 | ++ |
662 | ++ node = g_dbus_node_info_new_for_xml (systemd_iface, NULL); |
663 | ++ g_assert( node ); |
664 | ++ manager_info = g_dbus_node_info_lookup_interface (node, "org.freedesktop.systemd1.Manager"); |
665 | ++ g_assert( manager_info); |
666 | ++ |
667 | + g_bus_own_name (G_BUS_TYPE_SYSTEM, |
668 | + "org.freedesktop.systemd1", |
669 | + G_BUS_NAME_OWNER_FLAGS_NONE, |
670 | +@@ -238,4 +399,6 @@ main (void) |
671 | + cgmanager_moveself(); |
672 | + while (1) |
673 | + g_main_context_iteration (NULL, TRUE); |
674 | ++ |
675 | ++ g_dbus_node_info_unref (node); |
676 | + } |
677 | +Index: systemd-shim/src/unit.c |
678 | +=================================================================== |
679 | +--- systemd-shim.orig/src/unit.c |
680 | ++++ systemd-shim/src/unit.c |
681 | +@@ -18,6 +18,7 @@ |
682 | + */ |
683 | + |
684 | + #include "unit.h" |
685 | ++#include <string.h> |
686 | + |
687 | + G_DEFINE_TYPE (Unit, unit, G_TYPE_OBJECT) |
688 | + |
689 | +@@ -31,6 +32,30 @@ unit_class_init (UnitClass *class) |
690 | + { |
691 | + } |
692 | + |
693 | ++static gchar * |
694 | ++cg_session_unstrip(const gchar *orig) |
695 | ++{ |
696 | ++ guint sessid; |
697 | ++ if (sscanf(orig, "session_2d%u", &sessid) != 1) |
698 | ++ return NULL; |
699 | ++ return g_strdup_printf("session-%u.scope", sessid); |
700 | ++} |
701 | ++ |
702 | ++Unit * |
703 | ++fake_unit (const gchar *object_path) |
704 | ++{ |
705 | ++ Unit *unit = NULL; |
706 | ++ gchar *sessionpath = strrchr(object_path, '/'); |
707 | ++ if (!sessionpath) |
708 | ++ return NULL; |
709 | ++ gchar *path = cg_session_unstrip(sessionpath+1); |
710 | ++ if (!path) |
711 | ++ return NULL; |
712 | ++ unit = cgroup_unit_new (path); |
713 | ++ g_free(path); |
714 | ++ return unit; |
715 | ++} |
716 | ++ |
717 | + Unit * |
718 | + lookup_unit (GVariant *parameters, |
719 | + GError **error) |
720 | +@@ -103,3 +128,11 @@ unit_stop (Unit *unit) |
721 | + |
722 | + return UNIT_GET_CLASS (unit)->stop (unit); |
723 | + } |
724 | ++ |
725 | ++void |
726 | ++unit_abandon (Unit *unit) |
727 | ++{ |
728 | ++ g_return_if_fail (unit != NULL); |
729 | ++ |
730 | ++ return UNIT_GET_CLASS (unit)->abandon (unit); |
731 | ++} |
732 | +Index: systemd-shim/src/unit.h |
733 | +=================================================================== |
734 | +--- systemd-shim.orig/src/unit.h |
735 | ++++ systemd-shim/src/unit.h |
736 | +@@ -35,14 +35,17 @@ typedef struct |
737 | + void (* start) (Unit *unit); |
738 | + void (* start_transient) (Unit *unit, GVariant *properties); |
739 | + void (* stop) (Unit *unit); |
740 | ++ void (* abandon) (Unit *unit); |
741 | + } UnitClass; |
742 | + |
743 | + GType unit_get_type (void); |
744 | + Unit *lookup_unit (GVariant *parameters, GError **error); |
745 | ++Unit *fake_unit (const gchar *object_path); |
746 | + const gchar *unit_get_state (Unit *unit); |
747 | + void unit_start_transient (Unit *unit, GVariant *properties); |
748 | + void unit_start (Unit *unit); |
749 | + void unit_stop (Unit *unit); |
750 | ++void unit_abandon (Unit *unit); |
751 | + |
752 | + Unit *ntp_unit_get (void); |
753 | + |
754 | |
755 | === added file 'debian/patches/series' |
756 | --- debian/patches/series 1970-01-01 00:00:00 +0000 |
757 | +++ debian/patches/series 2014-09-03 22:02:47 +0000 |
758 | @@ -0,0 +1,1 @@ |
759 | +0001-Implement-Manager-StopUnit-and-Scope-Abandon.patch |
This fix is also required for bug 1357252.
The code changes look fine to me (although I'm not very familiar with the codebase). Just a few queries:
- src/systemd-shim.c : There's a chunk of code that's #if-0'd out. Can this be removed?
- I was imagining when I heard that systemd-shim was now long-running that it would exit when the session exits but I now see that it starts on-demand and then stays running for the duration of the boot to allow it to service requests from multiple sessions. Now that it is maintaining state, what's the implication of it stopping on error and being restarted?
- Does the shim need an apparmor policy now it's long-running?