Light Display Manager

Merge lp:~townsend/lightdm/fix-plymouthd-spin into lp:~mir-team/lightdm/unity

  1. fix-plymouthd-spin
  2. Merge into unity
Proposed by Christopher Townsend
Status: Merged
Approved by: Robert Ancell
Approved revision: 1635
Merged at revision: 1636
Proposed branch: lp:~townsend/lightdm/fix-plymouthd-spin
Merge into: lp:~mir-team/lightdm/unity
Diff against target: 90 lines (+12/-25)
4 files modified
src/plymouth.c (+2/-22)
src/plymouth.h (+1/-1)
src/seat-unity.c (+8/-1)
src/xserver-local.c (+1/-1)
To merge this branch: bzr merge lp:~townsend/lightdm/fix-plymouthd-spin
Reviewer Review Type Date Requested Status
Robert Ancell Approve
Daniel van Vugt Needs Information
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+174874@code.launchpad.net

Commit message

* Revert the workaround introduced in revno. 1619.
* Add in handling of "plymouth quit" when using the compositor to send the plymouth commands to deactivate and quit. (LP: #1192051)

Description of the change

In Xmir, the compositor start up is supposed to handle the interaction with plymouth unlike X only in which xserver-local does this. However, it is lacking a call to plymouth_quit(), so plymouthd will spin forever since nothing told it to quit.

This fix uses the stopping_plymouth boolean to set when to run plymouth_quit(). I think this is a bug in upstream lightdm as well since nothing ever sets that boolean to TRUE, rendering this a useless variable.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1635
http://jenkins.qa.ubuntu.com/job/mir-team-lightdm-unity-ci/34/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-lightdm-unity-saucy-amd64-ci/15

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-lightdm-unity-ci/34/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I thought the absence of plymouth_quit wass correct. Lightdm is not meant to quit plymouth, only deactivate it...

I thought you should not want plymouth quit because we needed a faster and smoother transition on startup, which deactivate gives you(?). But I also can't see any reason why plymouthd should keep running, so quit might make sense.

review: Needs Information
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

See also: /etc/init/plymouth-stop.conf
which is intended to quit plymouth. But apparently not with lightdm...

Revision history for this message
Christopher Townsend (townsend) wrote :

@Daniel,

Thanks for looking over this. I also thought deactivate was a more graceful way of shutting down plymouth, but I've looked through the plymouth code and basically all deactivate does is stop displaying the splash screen. There is no code in the deactivate path to kill the plymouthd process.

Also, when using the X only setup, "plymouth quit" is indeed called by xserver-local in lightdm and this is what kills off the plymouthd process. So what I did is took how xserver-local handles that and apply it to when the compositor is handling the plymouth duties. I think this is a bug in upstream lightdm as well, but it was undiscovered since xserver-local has been handling these duties until now.

The reason your workaround appeared to work is that is actually caused plymouthd to segfault. After the first deactivate is done, some pointers are NULLed out and the subsequent call to deactivate dereferences the null pointer. I think that is a bug as well, but not this bug we are trying to fix:)

I hope this helps explain my findings and rationale for this MP. I also put a bit more detail in the bug itself about this.

Revision history for this message
Robert Ancell (robert-ancell) wrote :

Thanks Christopher! Looking into it you're completely right - the Unity seat was not behaving like the X seat. I've pushed this to trunk with a regression test that confirms and checks the behaviour (lp:~robert-ancell/lightdm/unity-plymouth-fix).

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/plymouth.c'
2--- src/plymouth.c 2013-07-01 03:40:17 +0000
3+++ src/plymouth.c 2013-07-15 21:50:44 +0000
4@@ -81,30 +81,10 @@
5 }
6
7 void
8-plymouth_deactivate (gboolean wait)
9+plymouth_deactivate (void)
10 {
11- gint attempt = 1;
12 is_active = FALSE;
13-
14- /*
15- * The "plymouth" command has no option like --wait-for-deactivation so
16- * we have to wait in a loop.
17- */
18- do
19- {
20- g_debug("Deactivating Plymouth (attempt #%d)", attempt);
21- attempt++;
22- plymouth_run_command ("deactivate", NULL);
23-
24- if (!wait)
25- return;
26-
27- sleep (1);
28- have_checked_active_vt = FALSE;
29- } while (plymouth_has_active_vt () && attempt <= 60);
30-
31- if (has_active_vt)
32- g_debug("Failed to deactivate Plymouth.");
33+ plymouth_run_command ("deactivate", NULL);
34 }
35
36 void
37
38=== modified file 'src/plymouth.h'
39--- src/plymouth.h 2013-07-01 03:40:17 +0000
40+++ src/plymouth.h 2013-07-15 21:50:44 +0000
41@@ -22,7 +22,7 @@
42
43 gboolean plymouth_has_active_vt (void);
44
45-void plymouth_deactivate (gboolean wait);
46+void plymouth_deactivate (void);
47
48 void plymouth_quit (gboolean retain_splash);
49
50
51=== modified file 'src/seat-unity.c'
52--- src/seat-unity.c 2013-07-01 05:10:33 +0000
53+++ src/seat-unity.c 2013-07-15 21:50:44 +0000
54@@ -146,6 +146,12 @@
55 close (fd);
56 }
57 }
58+
59+ if(seat->priv->stopping_plymouth)
60+ {
61+ seat->priv->stopping_plymouth = FALSE;
62+ plymouth_quit(TRUE);
63+ }
64 }
65
66 static void
67@@ -309,8 +315,9 @@
68 if (active_vt >= vt_get_min ())
69 {
70 g_debug ("Compositor will replace Plymouth");
71+ SEAT_UNITY (seat)->priv->stopping_plymouth = TRUE;
72 SEAT_UNITY (seat)->priv->vt = active_vt;
73- plymouth_deactivate (TRUE);
74+ plymouth_deactivate ();
75 }
76 else
77 g_debug ("Plymouth is running on VT %d, but this is less than the configured minimum of %d so not replacing it", active_vt, vt_get_min ());
78
79=== modified file 'src/xserver-local.c'
80--- src/xserver-local.c 2013-07-01 03:40:17 +0000
81+++ src/xserver-local.c 2013-07-15 21:50:44 +0000
82@@ -170,7 +170,7 @@
83 g_debug ("X server %s will replace Plymouth", xserver_get_address (XSERVER (self)));
84 self->priv->replacing_plymouth = TRUE;
85 self->priv->vt = active_vt;
86- plymouth_deactivate (FALSE);
87+ plymouth_deactivate ();
88 }
89 else
90 g_debug ("Plymouth is running on VT %d, but this is less than the configured minimum of %d so not replacing it", active_vt, vt_get_min ());

Subscribers

People subscribed via source and target branches