[systemd-devel] [PATCH 2/2] backlight: Avoid restoring brightness to an unreadably dim level

[Josh Triplett]

On Wed, Mar 12, 2014 at 03:32:47AM +0100, Lennart Poettering wrote:
> On Tue, 11.03.14 18:55, Josh Triplett (josh at joshtriplett.org) wrote:
> 
> > +                /* Some systems turn the backlight all the way off at the
> > +                 * lowest levels.  Clamp saved brightness to at least 1 or 5%
> > +                 * of max_brightness.  This avoids preserving an unreadably dim
> > +                 * screen, which would otherwise force the user to disable
> > +                 * state restoration.
> > +                 */
> > +                max_brightness_str = udev_device_get_sysattr_value(device, "max_brightness");
> > +                if (!max_brightness_str) {
> > +                        log_warning("Failed to read max_brightness attribute; not checking saved brightness");
> > +                } else {
> 
> We try to avoid unnnecessary {} for single-line if blocks, if we can...

Even when the else block *does* need the braces?  (Note that kernel
style says to avoid braces on single-line blocks but always use braces
on all branches of an if/else-if/else if any branch needs them.)

> Hmmm, could you maybe move the entire clamping thing into a function of
> its own?  Maybe clamp_brightness(struct udev_device *d, char
> **brightness) or so, that simply patches the brightness string if it
> feels like it, otherwise leaves it unmodified?

Sure.

> > +                        unsigned long long brightness,
> > max_brightness, new_brightness;
> 
> Wow, you expect a lot of brightness levels! ;-)
> 
> I'd just stick to "unsigned" here... Keeps it more readable...

It's just two words ("long long") and a couple of "ll"s later, but OK.

> > +
> > +                        r = safe_atollu(value, &brightness);
> > +                        if (r < 0) {
> > +                                log_error("Failed to parse brightness \"%s\": %s", value, strerror(-r));
> > +                                return EXIT_FAILURE;
> > +                        }
> > +
> > +                        r = safe_atollu(max_brightness_str, &max_brightness);
> > +                        if (r < 0) {
> > +                                log_error("Failed to parse max_brightness \"%s\": %s", max_brightness_str, strerror(-r));
> > +                                return EXIT_FAILURE;
> > +                        }
> 
> Hmm, I'd prefer if the whole clamping business would be
> non-fatal. i.e. it clamps if it can read the files and parse them, but
> if it can't it won't do anything...

I thought about that, but it would have complicated the logic, and those
kernel files should always be numbers anyway.  However, with a move to a
separate function, this gets easier with early return, so sure.

> > +                        new_brightness = MAX3(brightness, 1, max_brightness/20);
> > +                        if (new_brightness != brightness) {
> > +                                _cleanup_free_ char *old_value = value;
> > +
> > +                                value = asprintf("%llu",
> > new_brightness);
> 
> asprintf() works differently... r = asprintf(&value, "%llu", new_brightness)...

Gah, I fixed that and then managed to send the wrong patch, sorry.

- Josh Triplett