Created attachment 123834 [details] [review]
UpKbdBacklight: always try to fetch the current brightness value on get

When GetBrightness is called it's better to fetch the current value in the
sysnode since many devices doesn't support the emission of brightness
changes to userland, and this could make settings daemons to handle
this value incorrectly

Comment on attachment 123834 [details] [review]
UpKbdBacklight: always try to fetch the current brightness value on get

I do agree to the general approach. Reading the brightness from sysfs is really quick, and it shouldn't be read very often.

But the actual patch looks too much like "bolted on the side" TBH, duplicates the code for reading the brightness, and leaves the "else" path meaningless as it won't ever return anything else than 0. Let's please clean this up:

 * Drop the "brightness" field from UpKbdBacklightPrivate.
 * In up_kbd_backlight_brightness_write(), drop setting priv->brightness.
 * Move the code for reading the file into a private static function and drop it from up_kbd_backlight_find(). Call it from _find() and _get_brightness().

Thanks!

Created attachment 123894 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysnode

When GetBrightness is called it's better to fetch the current value in the
sysnode since many devices doesn't support the emission of brightness
changes to userland, and this could make settings daemons to handle
this value incorrectly.

Added static up_kbd_backlight_brightness_read that is now called
also in initialization, it returns -1 on errors, and use it everywhere we need
to read or check the current brightness.

(In reply to Martin Pitt from comment #2)
> Comment on attachment 123834 [details] [review] [review]
> UpKbdBacklight: always try to fetch the current brightness value on get
> 
> I do agree to the general approach. Reading the brightness from sysfs is
> really quick, and it shouldn't be read very often.

Good.

> But the actual patch looks too much like "bolted on the side" TBH,
> duplicates the code for reading the brightness, and leaves the "else" path
> meaningless as it won't ever return anything else than 0.

Thanks for the review Martin, I've updated the patch to follow your comments, I also made the dbus Get method to return an error in case of invalid reads.

Cheers

Comment on attachment 123894 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysnode

Review of attachment 123894 [details] [review]:
-----------------------------------------------------------------

I like the general structure of this a lot more, thanks! I have some nitpicks below.

Please change the commit message like s/sysnode/sysfs attribute/, and s/Added/Add/.

::: src/up-kbd-backlight.c
@@ +62,5 @@
> +	if (kbd_backlight->priv->fd < 0) {
> +		g_warning ("cannot read kbd_backlight as file not open");
> +		goto out;
> +	}
> +

Please use g_return_val_if_fail(kbd_backlight->priv->fd >= 0, -1), as this should be an internal assertion. This also avoids the "goto", and you can drop the -1 initialization.

@@ +67,5 @@
> +	lseek (kbd_backlight->priv->fd, 0, SEEK_SET);
> +	len = read (kbd_backlight->priv->fd, buf, G_N_ELEMENTS (buf));
> +
> +	if (len > 0) {
> +		buf[MIN(len, (int) G_N_ELEMENTS (buf) - 1)] = '\0';

If you only read n-1 elements in the read() call, then you don't need this MIN dance and can just set buf[len] = '\0'.

@@ +72,5 @@
> +		brightness = g_ascii_strtoll (buf, &end, 10);
> +
> +		if (brightness < 0 ||
> +		    brightness > kbd_backlight->priv->max_brightness ||
> +		    (brightness == 0 && end == buf)) {

A parse error would be indicated by *end != '\0', i. e. there are leftover chars in the string that aren't numeric. "end == buf" should only happen if there are no characters at all, but I think this should be a bit more robust. So how about

   [...] || end == buf || *end != '\0'

?

@@ +92,4 @@
>  	gchar *text = NULL;
>  	gint retval;
>  	gint length;
> +	gint brightness;

This seems to be unused.

Comment on attachment 123894 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysnode

Review of attachment 123894 [details] [review]:
-----------------------------------------------------------------

Thanks for the review again, I'm attaching a new revision of the patch in a sec.

::: src/up-kbd-backlight.c
@@ +62,5 @@
> +	if (kbd_backlight->priv->fd < 0) {
> +		g_warning ("cannot read kbd_backlight as file not open");
> +		goto out;
> +	}
> +

Yeah, I do agree...  I quite hate using labels and goto, I just used that since it was somewhat consistent with the rest of the code.
But I would have done the same, although I also liked the idea of being more informative on errors (indeed it's implicit that if the fd is invalid, then there has been an open error, but still...).

Also, I've to keep the initialization to -1 around, or we could return an un-initialized value if len = 0 (i.e. if read returns 0).
Anyway I find this more visible than explicitly including it into the g_return_val_if_fail statement.

@@ +67,5 @@
> +	lseek (kbd_backlight->priv->fd, 0, SEEK_SET);
> +	len = read (kbd_backlight->priv->fd, buf, G_N_ELEMENTS (buf));
> +
> +	if (len > 0) {
> +		buf[MIN(len, (int) G_N_ELEMENTS (buf) - 1)] = '\0';

Sure.

@@ +72,5 @@
> +		brightness = g_ascii_strtoll (buf, &end, 10);
> +
> +		if (brightness < 0 ||
> +		    brightness > kbd_backlight->priv->max_brightness ||
> +		    (brightness == 0 && end == buf)) {

Sure, adding *end != '\0' allows to be even stricter... And it's fine from my POV.
However as per g_ascii_strtoll docs, "end == buf" also happens on generic parsing errors (together with "brightness == 0"), that's why I added that, and it should already cover all the cases. So are we sure we need the *end != '\0'?

I'm adding this in the new patch, anyway.

@@ +92,4 @@
>  	gchar *text = NULL;
>  	gint retval;
>  	gint length;
> +	gint brightness;

Yeah, sorry... An unnoticed leftover.

Created attachment 123933 [details] [review]
UpKbdBacklight: don't cache the brightness level, always read it from sysfs attrib

When GetBrightness is called it's better to fetch the current value in the
sysfs attribute since many devices doesn't support the emission of brightness
changes to userland, and this could make settings daemons to handle
this value incorrectly.

Add static up_kbd_backlight_brightness_read that is now called during
initialization too, it returns -1 on errors, and use it everywhere we need
to read or check the current brightness.

Perfect, thank you!

I applied this with a minor whitespace fix:

.git/rebase-apply/patch:61: space before tab in indent.

Hi,

(In reply to Marco Trevisan (Treviño) from comment #0)
> So, while it would be nice to have a KEY_KBDILLUMCHANGENOTIFY event or
> something similar (an udev event?) to be emitted by the kernel in such
> cases, this is not something easily addressable right now.

So while working on the kbd backlight control on my Dell XPS 15, not really playing well together with gnome, I hit the same problem. So I've ended up writing some kernel patches to allow poll() on the sysfs brightness attribute (there is a standard way to do poll on sysfs attributes):

http://www.spinics.net/lists/platform-driver-x86/msg09612.html

This is v2, v1 has received a somewhat favorable review (some changes were needed but no blockers)
so I expect this to go upstream, but you should probably wait with merging the 2nd patch till this is at least acked upstream. Note that without the kernel changes / on older kernels the poll() will simply never wakeup so this is safe to do on older kernels.

FYI: I've also implemented actually waking up the poll when a hardwired hotkey changes the keyboard backlight brightness for dell laptops:
http://www.spinics.net/lists/platform-driver-x86/msg09615.html
http://www.spinics.net/lists/platform-driver-x86/msg09613.html

Next on my list is adding support for this for Thinkpads.

I've also submitted a patch to make upower use poll(), see bug 98404.

As well as some gnome-settings-daemon fixes + support for monitoring for changes through upower:
https://bugzilla.gnome.org/show_bug.cgi?id=773402
https://bugzilla.gnome.org/show_bug.cgi?id=773403
https://bugzilla.gnome.org/show_bug.cgi?id=773405

Regards,

Hans