On Wed, Mar 12, 2014 at 03:32:47AM +0100, Lennart Poettering wrote:
> 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...
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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
