On Tue, 11.03.14 18:55, Josh Triplett (j...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel