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

[Lennart Poettering]

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...

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?

> +                        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...

> +
> +                        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...

> +
> +                        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)...


Lennart

-- 
Lennart Poettering, Red Hat