acpi-support should use ck-list-sessions to determine active X display and X user

Bug #933626 reported by Ayan George
18
This bug affects 1 person
Affects Status Importance Assigned to Milestone
acpi-support (Ubuntu)
High
Steve Langasek
Oneiric
Undecided
Unassigned
Precise
High
Steve Langasek

Bug Description

SRU Justificaiton
-----------------

Impact:

This bug impacts all Oneiric systems which use LightDM and are affected by lp870297 (Lightdm logins not being logged in wtmp).

In Oneiric, acpi-support depends on wtmp entries to determine the active console with getXconsole and getXuser. Since LightDM doesn't write to wtmp, this affects all systems that use acpi-support scripts to handle ACPI hotkey events -- including yet to be released hardware in the enablement/certification queue.

Fix:

The fix is to backport the changes made in Precise to Oneiric. This includes implementing getXuser and getXconsole with ConsoleKit (ck-list-sessions).

Patch:

This change is already in acpi-support .139:

  http://launchpadlibrarian.net/95198647/acpi-support_0.138_0.139.diff.gz

I've attached a debdiff that backports that change to acpi-support .139 in Oneiric.

Regression Potential:

There should be very little potential for regression. In fact, this should fix machines affected by lp87029, including OEM systems.

This has been tested using the following PPA:

  https://launchpad.net/~ayan/+archive/acpi-support-sru

------------------------

The acpi-support scripts use 'ps' and 'who' output to determine the current active X display and X user (in getXconsole and getXuser). Currently, LightDM does not write to utmp so 'who' does not output the correct information breaking getXconsole and getXuser.

ConsoleKit properly maintains the state of the console and probably should be used instead.

I've attached a patch that re-implements getXconsole and getXuser by calling ck-list-sessions.

Revision history for this message
Ayan George (ayan) wrote :
Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "power-funcs.diff" of this bug report has been identified as being a patch. The ubuntu-reviewers team has been subscribed to the bug report so that they can review the patch. In the event that this is in fact not a patch you can resolve this situation by removing the tag 'patch' from the bug report and editing the attachment so that it is not flagged as a patch. Additionally, if you are member of the ubuntu-reviewers team please also unsubscribe the team from this bug report.

[This is an automated message performed by a Launchpad user owned by Brian Murray. Please contact him regarding any issues with the action taken in this bug report.]

tags: added: patch
Ayan George (ayan)
Changed in acpi-support (Ubuntu):
assignee: nobody → Steve Langasek (vorlon)
Revision history for this message
Steve Langasek (vorlon) wrote :

Thanks for the patch! some feedback:

- the check for x11-display matches all sessions including console sessions, because x11-display is always set. We need to specifically check for a non-empty value here.
- the rotatescreen.sh script specifically assumes that getXconsole() can be applied once for each value of $displaynum, not just the currently active session. This is already broken and we should probably clean it up.
- sleep.sh, screenblank.sh and lid.sh assume that they can use getXuser() to get the user for *each* X display so that they can lock the screen (e.g., before suspending). This is important to preserve, since there may indeed be multiple X sessions running simultaneously (guest sessions, etc). This means we also need to be able to match on non-active sessions.
- the && unix_user in the check returns false if root is logged in to an X session. While this is discouraged and unlikely, we don't really want to fail to handle such sessions. Fixed by omitting this part of the check.
- the $user variable set by getXuser is used elsewhere as the argument to su, but the current check returns a numeric uid which doesn't work. We need to translate this to a username to retain compatibility.

I've adjusted the patch accordingly and committed it to the bzr repo. Will upload this to precise shortly.

Changed in acpi-support (Ubuntu):
status: New → In Progress
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package acpi-support - 0.139

---------------
acpi-support (0.139) precise; urgency=low

  * lib/power-funcs: adjust getXuser and getXconsole to use consolekit
    instead of ps/who to identify the "foreground" session and the owner
    of a given X session, since lightdm is currently failing to record this
    information in utmp. Also, add headers for each of these functions
    documenting how they're supposed to work, since it's completely
    non-obvious and some of the scripts already in acpi-support are using
    them wrong. Thanks to Ayan George <email address hidden> for the
    preliminary patch. LP: #933626, #893271.
  * rotatescreen.sh: fix wrong use of getXconsole where getXuser is needed.
 -- Steve Langasek <email address hidden> Sat, 03 Mar 2012 15:49:47 -0800

Changed in acpi-support (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
Axel Beckert (xtaran) wrote :

Meh, now this pulls in consolekit on lightweight or non-X Debian installations, too.

Can't the consolekit usage be made optional?

Revision history for this message
Ayan George (ayan) wrote :

Debdiff from acpi-support .138 to .138-ubuntu1

description: updated
Ayan George (ayan)
tags: added: blocks-hwcert-enablement
Changed in acpi-support (Ubuntu):
milestone: none → oneiric-updates
description: updated
Revision history for this message
Steve Langasek (vorlon) wrote :

> Debdiff from acpi-support .138 to .138-ubuntu1

acpi-support is an Ubuntu native package. Please use 0.138-0.1 instead as an SRU version number.

Revision history for this message
Martin Pitt (pitti) wrote :

Uploaded with a more meaningful changelog, fixed LP bug refs, and fixed version number. Thanks!

Changed in acpi-support (Ubuntu Oneiric):
status: New → Fix Committed
Revision history for this message
Clint Byrum (clint-fewbar) wrote : Please test proposed package

Hello Ayan, or anyone else affected,

Accepted acpi-support into oneiric-proposed. The package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

tags: added: verification-needed
Revision history for this message
Ayan George (ayan) wrote :

Thank you Clint!

I'll test on Monday.

Revision history for this message
James M. Leddy (jm-leddy) wrote :

Hi Ayan, what were the results of your test?

Revision history for this message
Ayan George (ayan) wrote :

Verified that this works on a Sutton machine.

This does require a change to an OEM script that locally implements the new getXuser though. I'll let the OEM team know.

Martin Pitt (pitti)
tags: added: verification-done
removed: verification-needed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package acpi-support - 0.138.1

---------------
acpi-support (0.138.1) oneiric-proposed; urgency=low

  * Added lenovo-touchpad2 from precise acpi-support-0.140 which supports
    newer thinkpads that are in the certification process.
  * Backported power-funcs from precise acpi-support-0.140:
    - Use ck-list-sessions to determine active X display and X user. This
      works around lightdm not always recording logins in wtmp (LP #870297).
      (LP: #933626)
 -- Ayan George <email address hidden> Tue, 03 Apr 2012 11:45:09 -0400

Changed in acpi-support (Ubuntu Oneiric):
status: Fix Committed → Fix Released
Revision history for this message
Iustin Pop (iustin) wrote :

I concur with comment #5. This pulls in consolekit on many non-X installations.

Is there a technical reason why lightdm can't be fixed to properly log to wtmp? Or should the acpi-support-base be split into a "base" package (no heavyweight dependencies) and acpi-support package? Well, we already have that. But I mean, keep the separation between very basic ACPI events and ones that depend/interact with a logged-in session.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers