After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748804 - Cannot change brightness from GDM
Cannot change brightness from GDM
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: power
3.16.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2015-05-02 15:59 UTC by Juraj Fiala
Modified: 2015-05-12 14:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
power: Refactor the backlight helper spawning (9.96 KB, patch)
2015-05-04 16:32 UTC, Rui Matos
none Details | Review
power: Don't pass SHELL in the backlight helper environment (1.71 KB, patch)
2015-05-04 16:32 UTC, Rui Matos
committed Details | Review
power: Refactor the backlight helper spawning (10.17 KB, patch)
2015-05-04 17:14 UTC, Rui Matos
committed Details | Review

Description Juraj Fiala 2015-05-02 15:59:35 UTC
Arch Linux (fully udpated), GNOME 3.16.1.

The slider is stuck between two values and brightness doesn't change.

Attempting to change the brightness produces this error (taken from Logs):

pkexec

Message: gdm: The value for the SHELL variable was not found the /etc/shells file [USER=root] [TTY=unknown] [CWD=/var/lib/gdm] [COMMAND=/usr/lib/gnome-settings-daemon/gsd-backlight-helper --set-brightness 18]

Priority: 2

Thanks.
Comment 1 Florian Müllner 2015-05-02 16:56:27 UTC
That looks like gnome-settings-daemon gets the request to change the brightness just fine, so the error is likely on the g-s-d side.
Comment 2 Rui Matos 2015-05-03 14:52:47 UTC
(In reply to Juraj Fiala from comment #0)
> pkexec
> 
> Message: gdm: The value for the SHELL variable was not found the /etc/shells

What's the contents of your /etc/shells? And the output of

$ getent passwd gdm

?
Comment 3 Juraj Fiala 2015-05-03 18:02:28 UTC
$ cat /etc/shells 
#
# /etc/shells
#

/bin/sh
/bin/bash

# End of file

$ getent passwd gdm
gdm:x:120:120:Gnome Display Manager:/var/lib/gdm:/sbin/nologin
Comment 4 Rui Matos 2015-05-04 16:32:12 UTC
Created attachment 302881 [details] [review]
power: Refactor the backlight helper spawning

We'll need to change the environment that we pass on to the helper and
this will make it easier.
Comment 5 Rui Matos 2015-05-04 16:32:29 UTC
Created attachment 302882 [details] [review]
power: Don't pass SHELL in the backlight helper environment

Doing so causes pkexec to fail[0] if the current user's SHELL doesn't
exist in /etc/shells which happens for the gdm user in some
distributions.

Since we don't need this environment variable in the helper we can
work around that by just unsetting it.

[0] http://cgit.freedesktop.org/polkit/commit/?id=5a388b6ebb095de6dc7f315b651a84fc31d268d7
Comment 6 Bastien Nocera 2015-05-04 16:38:23 UTC
Review of attachment 302881 [details] [review]:

Looks useful otherwise.

::: plugins/power/gpm-common.c
@@ +46,3 @@
 #define GSD_POWER_MANAGER_CRITICAL_ALERT_TIMEOUT  5 /* seconds */
 
+enum BHCommand {

No reason for the name to be this glib though.

@@ +406,3 @@
+        switch (command) {
+        case BH_GET:
+                argv[2] = "--get-brightness";

define a table with the arguments and do:
argv[2] = helper_args[command];
instead?
Comment 7 Bastien Nocera 2015-05-04 16:40:29 UTC
Review of attachment 302882 [details] [review]:

::: plugins/power/gpm-common.c
@@ +431,3 @@
         return g_spawn_sync (NULL,
                              command == BH_SET ? argv : &argv[1],
+                             get_backlight_helper_environ (),

Not sure that g_spawn_sync()'s environ is transfer full, so this might leak?
Comment 8 Rui Matos 2015-05-04 17:13:03 UTC
(In reply to Bastien Nocera from comment #7)
> Not sure that g_spawn_sync()'s environ is transfer full, so this might leak?

It's transfer none, but note that I'm using a static variable in get_backlight_helper_environ() which means that we keep the array around all the time, effectively making it a global singleton.

I did that to not have to build and filter out the variable from the array on every call.
Comment 9 Rui Matos 2015-05-04 17:14:11 UTC
Created attachment 302887 [details] [review]
power: Refactor the backlight helper spawning

--

(In reply to Bastien Nocera from comment #6)
> +enum BHCommand {
>
> No reason for the name to be this glib though.

:-) Ok, spelled it all out now

> define a table with the arguments and do:
> argv[2] = helper_args[command];
> instead?

sure
Comment 10 Rui Matos 2015-05-12 14:31:04 UTC
Attachment 302882 [details] pushed as e9edfbb - power: Don't pass SHELL in the backlight helper environment
Attachment 302887 [details] pushed as d1329f9 - power: Refactor the backlight helper spawning