Light Display Manager

Merge lp:~vanvugt/lightdm/fix-1192051 into lp:~mir-team/lightdm/unity

  1. fix-1192051
  2. Merge into unity
Proposed by Daniel van Vugt
Status: Merged
Approved by: Robert Ancell
Approved revision: 1618
Merged at revision: 1619
Proposed branch: lp:~vanvugt/lightdm/fix-1192051
Merge into: lp:~mir-team/lightdm/unity
Diff against target: 75 lines (+25/-5)
4 files modified
src/plymouth.c (+22/-2)
src/plymouth.h (+1/-1)
src/seat-unity.c (+1/-1)
src/xserver-local.c (+1/-1)
To merge this branch: bzr merge lp:~vanvugt/lightdm/fix-1192051
Reviewer Review Type Date Requested Status
Robert Ancell Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+170362@code.launchpad.net

Commit message

seat-unity: Wait for Plymouth to complete the "deactivate" command. Otherwise
we risk fighting for the VT device and Plymouth's error handling is not robust
enough to handle that, making it spin at 100% CPU and never deactivating.
(LP: #1192051)

Description of the change

I'm sure some kind of robustness fix could be done in plymouth instead, but:

1. I don't yet understand the full flow of this bug from lightdm to plymouth. Other than plymouth is spinning on an fd of "/dev/tty7" with errno==EIO. And waiting for the plymouth deactivate to properly complete (hence drmDropMaster), avoids the bug.

2. Our PPAs don't contain plymouth so packaging the fix there would take significantly more effort.

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

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

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

review: Approve (continuous-integration)
Revision history for this message
Robert Ancell (robert-ancell) wrote :

Hacky, but will work for now.

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

I agree it's hacky. And I spent a couple of extra hours trying to find a way to make it less so. This is the least of the evils.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/plymouth.c'
--- src/plymouth.c 2011-09-07 05:37:55 +0000
+++ src/plymouth.c 2013-06-19 13:42:27 +0000
@@ -81,10 +81,30 @@
81}81}
8282
83void83void
84plymouth_deactivate (void)84plymouth_deactivate (gboolean wait)
85{85{
86 gint attempt = 1;
86 is_active = FALSE;87 is_active = FALSE;
87 plymouth_run_command ("deactivate", NULL);88
89 /*
90 * The "plymouth" command has no option like --wait-for-deactivation so
91 * we have to wait in a loop.
92 */
93 do
94 {
95 g_debug("Deactivating Plymouth (attempt #%d)", attempt);
96 attempt++;
97 plymouth_run_command ("deactivate", NULL);
98
99 if (!wait)
100 return;
101
102 sleep (1);
103 have_checked_active_vt = FALSE;
104 } while (plymouth_has_active_vt () && attempt <= 60);
105
106 if (has_active_vt)
107 g_debug("Failed to deactivate Plymouth.");
88}108}
89109
90void110void
91111
=== modified file 'src/plymouth.h'
--- src/plymouth.h 2013-04-23 03:07:03 +0000
+++ src/plymouth.h 2013-06-19 13:42:27 +0000
@@ -22,7 +22,7 @@
2222
23gboolean plymouth_has_active_vt (void);23gboolean plymouth_has_active_vt (void);
2424
25void plymouth_deactivate (void);25void plymouth_deactivate (gboolean wait);
2626
27void plymouth_quit (gboolean retain_splash);27void plymouth_quit (gboolean retain_splash);
2828
2929
=== modified file 'src/seat-unity.c'
--- src/seat-unity.c 2013-06-19 04:32:53 +0000
+++ src/seat-unity.c 2013-06-19 13:42:27 +0000
@@ -305,7 +305,7 @@
305 {305 {
306 g_debug ("Compositor will replace Plymouth");306 g_debug ("Compositor will replace Plymouth");
307 SEAT_UNITY (seat)->priv->vt = active_vt;307 SEAT_UNITY (seat)->priv->vt = active_vt;
308 plymouth_deactivate ();308 plymouth_deactivate (TRUE);
309 }309 }
310 else310 else
311 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 ());311 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 ());
312312
=== modified file 'src/xserver-local.c'
--- src/xserver-local.c 2013-06-17 23:38:35 +0000
+++ src/xserver-local.c 2013-06-19 13:42:27 +0000
@@ -170,7 +170,7 @@
170 g_debug ("X server %s will replace Plymouth", xserver_get_address (XSERVER (self)));170 g_debug ("X server %s will replace Plymouth", xserver_get_address (XSERVER (self)));
171 self->priv->replacing_plymouth = TRUE;171 self->priv->replacing_plymouth = TRUE;
172 self->priv->vt = active_vt;172 self->priv->vt = active_vt;
173 plymouth_deactivate ();173 plymouth_deactivate (FALSE);
174 }174 }
175 else175 else
176 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 ());176 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