From 0e1564a261f4af1f17f01806769291656e15e759 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Dec 2023 04:17:06 +0900 Subject: [PATCH 1/6] backlight: move validity check of max_brightness to get_max_brightness() Also rename get_max_brightness() -> read_max_brightness() for consistency with read_brightness(). --- src/backlight/backlight.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c index 5ac9f904a9..363ed38005 100644 --- a/src/backlight/backlight.c +++ b/src/backlight/backlight.c @@ -309,7 +309,8 @@ static int validate_device(sd_device *device) { return true; } -static int get_max_brightness(sd_device *device, unsigned *ret) { +static int read_max_brightness(sd_device *device, unsigned *ret) { + unsigned max_brightness; const char *s; int r; @@ -320,11 +321,22 @@ static int get_max_brightness(sd_device *device, unsigned *ret) { if (r < 0) return log_device_warning_errno(device, r, "Failed to read 'max_brightness' attribute: %m"); - r = safe_atou(s, ret); + r = safe_atou(s, &max_brightness); if (r < 0) return log_device_warning_errno(device, r, "Failed to parse 'max_brightness' \"%s\": %m", s); - return 0; + /* If max_brightness is 0, then there is no actual backlight device. This happens on desktops + * with Asus mainboards that load the eeepc-wmi module. */ + if (max_brightness == 0) { + log_device_warning(device, "Maximum brightness is 0, ignoring device."); + *ret = 0; + return 0; + } + + log_device_debug(device, "Maximum brightness is %u", max_brightness); + + *ret = max_brightness; + return 1; /* valid max brightness */ } static int clamp_brightness( @@ -506,17 +518,9 @@ static int run(int argc, char *argv[]) { return ignore ? 0 : r; } - /* If max_brightness is 0, then there is no actual backlight device. This happens on desktops - * with Asus mainboards that load the eeepc-wmi module. */ - if (get_max_brightness(device, &max_brightness) < 0) - return 0; - - if (max_brightness == 0) { - log_device_warning(device, "Maximum brightness is 0, ignoring device."); - return 0; - } - - log_device_debug(device, "Maximum brightness is %u", max_brightness); + r = read_max_brightness(device, &max_brightness); + if (r <= 0) + return r; escaped_ss = cescape(ss); if (!escaped_ss) From 7135e6291df3a82777bc4ed73ba59c16fe2c2a4b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Dec 2023 04:21:25 +0900 Subject: [PATCH 2/6] backlight: split out build_save_file_path() No functional change, just refactoring. --- src/backlight/backlight.c | 62 +++++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c index 363ed38005..039f9a8ac9 100644 --- a/src/backlight/backlight.c +++ b/src/backlight/backlight.c @@ -471,10 +471,49 @@ use_brightness: return 0; } +static int build_save_file_path(sd_device *device, char **ret) { + _cleanup_free_ char *escaped_subsystem = NULL, *escaped_sysname = NULL, *path = NULL; + const char *s; + int r; + + assert(device); + assert(ret); + + r = sd_device_get_subsystem(device, &s); + if (r < 0) + return log_device_error_errno(device, r, "Failed to get subsystem: %m"); + + escaped_subsystem = cescape(s); + if (!escaped_subsystem) + return log_oom(); + + r = sd_device_get_sysname(device, &s); + if (r < 0) + return log_device_error_errno(device, r, "Failed to get sysname: %m"); + + escaped_sysname = cescape(s); + if (!escaped_sysname) + return log_oom(); + + if (sd_device_get_property_value(device, "ID_PATH", &s) >= 0) { + _cleanup_free_ char *escaped_path_id = cescape(s); + if (!escaped_path_id) + return log_oom(); + + path = strjoin("/var/lib/systemd/backlight/", escaped_path_id, ":", escaped_subsystem, ":", escaped_sysname); + } else + path = strjoin("/var/lib/systemd/backlight/", escaped_subsystem, ":", escaped_sysname); + if (!path) + return log_oom(); + + *ret = TAKE_PTR(path); + return 0; +} + static int run(int argc, char *argv[]) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; - _cleanup_free_ char *escaped_ss = NULL, *escaped_sysname = NULL, *escaped_path_id = NULL; - const char *sysname, *path_id, *ss, *saved; + _cleanup_free_ char *saved = NULL; + const char *sysname, *ss; unsigned max_brightness, brightness; int r; @@ -522,22 +561,9 @@ static int run(int argc, char *argv[]) { if (r <= 0) return r; - escaped_ss = cescape(ss); - if (!escaped_ss) - return log_oom(); - - escaped_sysname = cescape(sysname); - if (!escaped_sysname) - return log_oom(); - - if (sd_device_get_property_value(device, "ID_PATH", &path_id) >= 0) { - escaped_path_id = cescape(path_id); - if (!escaped_path_id) - return log_oom(); - - saved = strjoina("/var/lib/systemd/backlight/", escaped_path_id, ":", escaped_ss, ":", escaped_sysname); - } else - saved = strjoina("/var/lib/systemd/backlight/", escaped_ss, ":", escaped_sysname); + r = build_save_file_path(device, &saved); + if (r < 0) + return r; /* If there are multiple conflicting backlight devices, then their probing at boot-time might * happen in any order. This means the validity checking of the device then is not reliable, From 69ba99f9f33e2e8f5e603257baedd8e500b41d43 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Dec 2023 04:24:31 +0900 Subject: [PATCH 3/6] backlight: split out device_new_from_arg() While at it, this replaces strndupa_safe() with strndup(), as the input is a user-controlled string. No functional change, just refactoring. --- src/backlight/backlight.c | 66 +++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c index 039f9a8ac9..c988a7bd14 100644 --- a/src/backlight/backlight.c +++ b/src/backlight/backlight.c @@ -510,10 +510,49 @@ static int build_save_file_path(sd_device *device, char **ret) { return 0; } +static int device_new_from_arg(const char *s, sd_device **ret) { + _cleanup_(sd_device_unrefp) sd_device *device = NULL; + _cleanup_free_ char *subsystem = NULL; + const char *sysname; + int r; + + assert(s); + assert(ret); + + sysname = strchr(s, ':'); + if (!sysname) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Requires a subsystem and sysname pair specifying a backlight or LED device."); + + subsystem = strndup(s, sysname - s); + if (!subsystem) + return log_oom(); + + sysname++; + + if (!STR_IN_SET(subsystem, "backlight", "leds")) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Not a backlight or LED device: '%s:%s'", + subsystem, sysname); + + r = sd_device_new_from_subsystem_sysname(&device, subsystem, sysname); + if (r == -ENODEV) { + /* Some drivers, e.g. for AMD GPU, removes acpi backlight device soon after it is added. + * See issue #21997. */ + log_debug_errno(r, "Failed to get backlight or LED device '%s:%s', ignoring: %m", subsystem, sysname); + *ret = NULL; + return 0; + } + if (r < 0) + return log_error_errno(r, "Failed to get backlight or LED device '%s:%s': %m", subsystem, sysname); + + *ret = TAKE_PTR(device); + return 1; /* Found. */ +} + static int run(int argc, char *argv[]) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; _cleanup_free_ char *saved = NULL; - const char *sysname, *ss; unsigned max_brightness, brightness; int r; @@ -534,28 +573,9 @@ static int run(int argc, char *argv[]) { if (r < 0) return log_error_errno(r, "Failed to create backlight directory /var/lib/systemd/backlight: %m"); - sysname = strchr(argv[2], ':'); - if (!sysname) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Requires a subsystem and sysname pair specifying a backlight device."); - - ss = strndupa_safe(argv[2], sysname - argv[2]); - - sysname++; - - if (!STR_IN_SET(ss, "backlight", "leds")) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Not a backlight or LED device: '%s:%s'", ss, sysname); - - r = sd_device_new_from_subsystem_sysname(&device, ss, sysname); - if (r < 0) { - bool ignore = r == -ENODEV; - - /* Some drivers, e.g. for AMD GPU, removes acpi backlight device soon after it is added. - * See issue #21997. */ - log_full_errno(ignore ? LOG_DEBUG : LOG_ERR, r, - "Failed to get backlight or LED device '%s:%s'%s: %m", - ss, sysname, ignore ? ", ignoring" : ""); - return ignore ? 0 : r; - } + r = device_new_from_arg(argv[2], &device); + if (r <= 0) + return r; r = read_max_brightness(device, &max_brightness); if (r <= 0) From 78b4ff5df454e1911576b2dd86f437d26185ba6f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Dec 2023 04:29:54 +0900 Subject: [PATCH 4/6] backlight: split out read_saved_brightness() No functional change, just refactoring. --- src/backlight/backlight.c | 66 ++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 26 deletions(-) diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c index c988a7bd14..4298654479 100644 --- a/src/backlight/backlight.c +++ b/src/backlight/backlight.c @@ -510,6 +510,37 @@ static int build_save_file_path(sd_device *device, char **ret) { return 0; } +static int read_saved_brightness(sd_device *device, unsigned *ret) { + _cleanup_free_ char *path = NULL, *value = NULL; + int r; + + assert(device); + assert(ret); + + r = build_save_file_path(device, &path); + if (r < 0) + return r; + + r = read_one_line_file(path, &value); + if (r < 0) { + if (r != -ENOENT) + log_device_error_errno(device, r, "Failed to read %s: %m", path); + return r; + } + + r = safe_atou(value, ret); + if (r < 0) { + log_device_warning_errno(device, r, + "Failed to parse saved brightness '%s', removing %s.", + value, path); + (void) unlink(path); + return r; + } + + log_device_debug(device, "Using saved brightness %u.", *ret); + return 0; +} + static int device_new_from_arg(const char *s, sd_device **ret) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; _cleanup_free_ char *subsystem = NULL; @@ -552,7 +583,6 @@ static int device_new_from_arg(const char *s, sd_device **ret) { static int run(int argc, char *argv[]) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; - _cleanup_free_ char *saved = NULL; unsigned max_brightness, brightness; int r; @@ -581,10 +611,6 @@ static int run(int argc, char *argv[]) { if (r <= 0) return r; - r = build_save_file_path(device, &saved); - if (r < 0) - return r; - /* If there are multiple conflicting backlight devices, then their probing at boot-time might * happen in any order. This means the validity checking of the device then is not reliable, * since it might not see other devices conflicting with a specific backlight. To deal with @@ -592,7 +618,6 @@ static int run(int argc, char *argv[]) { * be complete), so that the validity check at boot time doesn't have to be reliable. */ if (streq(argv[1], "load")) { - _cleanup_free_ char *value = NULL; unsigned percent; bool clamp; @@ -604,25 +629,7 @@ static int run(int argc, char *argv[]) { clamp = shall_clamp(device, &percent); - r = read_one_line_file(saved, &value); - if (r < 0 && r != -ENOENT) - return log_error_errno(r, "Failed to read %s: %m", saved); - if (r > 0) { - r = safe_atou(value, &brightness); - if (r < 0) { - log_warning_errno(r, "Failed to parse saved brightness '%s', removing %s.", - value, saved); - (void) unlink(saved); - } else { - log_debug("Using saved brightness %u.", brightness); - if (clamp) - (void) clamp_brightness(device, percent, /* saved = */ true, max_brightness, &brightness); - - /* Do not fall back to read current brightness below. */ - r = 1; - } - } - if (r <= 0) { + if (read_saved_brightness(device, &brightness) < 0) { /* Fallback to clamping current brightness or exit early if clamping is not * supported/enabled. */ if (!clamp) @@ -633,13 +640,20 @@ static int run(int argc, char *argv[]) { return log_device_error_errno(device, r, "Failed to read current brightness: %m"); (void) clamp_brightness(device, percent, /* saved = */ false, max_brightness, &brightness); - } + } else if (clamp) + (void) clamp_brightness(device, percent, /* saved = */ true, max_brightness, &brightness); r = sd_device_set_sysattr_valuef(device, "brightness", "%u", brightness); if (r < 0) return log_device_error_errno(device, r, "Failed to write system 'brightness' attribute: %m"); } else if (streq(argv[1], "save")) { + _cleanup_free_ char *saved = NULL; + + r = build_save_file_path(device, &saved); + if (r < 0) + return r; + if (validate_device(device) == 0) { (void) unlink(saved); return 0; From 48de55c38cb4f4e286a321496a734bcbd66abded Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Dec 2023 09:42:45 +0900 Subject: [PATCH 5/6] backlight: use WRITE_STRING_FILE_MKDIR_0755 flag on save No functional change, just refactoring. --- src/backlight/backlight.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c index 4298654479..a12e3dede9 100644 --- a/src/backlight/backlight.c +++ b/src/backlight/backlight.c @@ -11,7 +11,6 @@ #include "escape.h" #include "fileio.h" #include "main-func.h" -#include "mkdir.h" #include "parse-util.h" #include "percent-util.h" #include "pretty-print.h" @@ -599,10 +598,6 @@ static int run(int argc, char *argv[]) { umask(0022); - r = mkdir_p("/var/lib/systemd/backlight", 0755); - if (r < 0) - return log_error_errno(r, "Failed to create backlight directory /var/lib/systemd/backlight: %m"); - r = device_new_from_arg(argv[2], &device); if (r <= 0) return r; @@ -663,7 +658,7 @@ static int run(int argc, char *argv[]) { if (r < 0) return log_device_error_errno(device, r, "Failed to read current brightness: %m"); - r = write_string_filef(saved, WRITE_STRING_FILE_CREATE, "%u", brightness); + r = write_string_filef(saved, WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_MKDIR_0755, "%u", brightness); if (r < 0) return log_device_error_errno(device, r, "Failed to write %s: %m", saved); From f8f59f32809ce87da7c0fccc47e9e1a040844182 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 22 Dec 2023 04:36:34 +0900 Subject: [PATCH 6/6] backlight: split out verb_load() and verb_save(), then use dispatch_verb() No functional change, just refactoring. --- src/backlight/backlight.c | 144 +++++++++++++++++++++----------------- 1 file changed, 81 insertions(+), 63 deletions(-) diff --git a/src/backlight/backlight.c b/src/backlight/backlight.c index a12e3dede9..2f893e9222 100644 --- a/src/backlight/backlight.c +++ b/src/backlight/backlight.c @@ -19,6 +19,7 @@ #include "string-util.h" #include "strv.h" #include "terminal-util.h" +#include "verbs.h" #define PCI_CLASS_GRAPHICS_CARD 0x30000 @@ -580,25 +581,18 @@ static int device_new_from_arg(const char *s, sd_device **ret) { return 1; /* Found. */ } -static int run(int argc, char *argv[]) { +static int verb_load(int argc, char *argv[], void *userdata) { _cleanup_(sd_device_unrefp) sd_device *device = NULL; - unsigned max_brightness, brightness; + unsigned max_brightness, brightness, percent; + bool clamp; int r; - log_setup(); + assert(argc == 2); - if (argv_looks_like_help(argc, argv)) - return help(); + if (!shall_restore_state()) + return 0; - if (argc != 3) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "This program requires two arguments."); - - if (!STR_IN_SET(argv[1], "load", "save")) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Unknown verb %s.", argv[1]); - - umask(0022); - - r = device_new_from_arg(argv[2], &device); + r = device_new_from_arg(argv[1], &device); if (r <= 0) return r; @@ -606,66 +600,90 @@ static int run(int argc, char *argv[]) { if (r <= 0) return r; - /* If there are multiple conflicting backlight devices, then their probing at boot-time might - * happen in any order. This means the validity checking of the device then is not reliable, - * since it might not see other devices conflicting with a specific backlight. To deal with - * this, we will actively delete backlight state files at shutdown (where device probing should - * be complete), so that the validity check at boot time doesn't have to be reliable. */ + /* Ignore any errors in validation, and use the device as is. */ + if (validate_device(device) == 0) + return 0; - if (streq(argv[1], "load")) { - unsigned percent; - bool clamp; + clamp = shall_clamp(device, &percent); - if (!shall_restore_state()) + r = read_saved_brightness(device, &brightness); + if (r < 0) { + /* Fallback to clamping current brightness or exit early if clamping is not + * supported/enabled. */ + if (!clamp) return 0; - if (validate_device(device) == 0) - return 0; - - clamp = shall_clamp(device, &percent); - - if (read_saved_brightness(device, &brightness) < 0) { - /* Fallback to clamping current brightness or exit early if clamping is not - * supported/enabled. */ - if (!clamp) - return 0; - - r = read_brightness(device, max_brightness, &brightness); - if (r < 0) - return log_device_error_errno(device, r, "Failed to read current brightness: %m"); - - (void) clamp_brightness(device, percent, /* saved = */ false, max_brightness, &brightness); - } else if (clamp) - (void) clamp_brightness(device, percent, /* saved = */ true, max_brightness, &brightness); - - r = sd_device_set_sysattr_valuef(device, "brightness", "%u", brightness); - if (r < 0) - return log_device_error_errno(device, r, "Failed to write system 'brightness' attribute: %m"); - - } else if (streq(argv[1], "save")) { - _cleanup_free_ char *saved = NULL; - - r = build_save_file_path(device, &saved); - if (r < 0) - return r; - - if (validate_device(device) == 0) { - (void) unlink(saved); - return 0; - } - r = read_brightness(device, max_brightness, &brightness); if (r < 0) return log_device_error_errno(device, r, "Failed to read current brightness: %m"); - r = write_string_filef(saved, WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_MKDIR_0755, "%u", brightness); - if (r < 0) - return log_device_error_errno(device, r, "Failed to write %s: %m", saved); + (void) clamp_brightness(device, percent, /* saved = */ false, max_brightness, &brightness); + } else if (clamp) + (void) clamp_brightness(device, percent, /* saved = */ true, max_brightness, &brightness); - } else - assert_not_reached(); + r = sd_device_set_sysattr_valuef(device, "brightness", "%u", brightness); + if (r < 0) + return log_device_error_errno(device, r, "Failed to write system 'brightness' attribute: %m"); return 0; } +static int verb_save(int argc, char *argv[], void *userdata) { + _cleanup_(sd_device_unrefp) sd_device *device = NULL; + _cleanup_free_ char *path = NULL; + unsigned max_brightness, brightness; + int r; + + assert(argc == 2); + + r = device_new_from_arg(argv[1], &device); + if (r <= 0) + return r; + + r = read_max_brightness(device, &max_brightness); + if (r <= 0) + return r; + + r = build_save_file_path(device, &path); + if (r < 0) + return r; + + /* If there are multiple conflicting backlight devices, then their probing at boot-time might + * happen in any order. This means the validity checking of the device then is not reliable, + * since it might not see other devices conflicting with a specific backlight. To deal with + * this, we will actively delete backlight state files at shutdown (where device probing should + * be complete), so that the validity check at boot time doesn't have to be reliable. */ + if (validate_device(device) == 0) { + (void) unlink(path); + return 0; + } + + r = read_brightness(device, max_brightness, &brightness); + if (r < 0) + return log_device_error_errno(device, r, "Failed to read current brightness: %m"); + + r = write_string_filef(path, WRITE_STRING_FILE_CREATE | WRITE_STRING_FILE_MKDIR_0755, "%u", brightness); + if (r < 0) + return log_device_error_errno(device, r, "Failed to write %s: %m", path); + + return 0; +} + +static int run(int argc, char *argv[]) { + static const Verb verbs[] = { + { "load", 2, 2, VERB_ONLINE_ONLY, verb_load }, + { "save", 2, 2, VERB_ONLINE_ONLY, verb_save }, + {} + }; + + log_setup(); + + if (argv_looks_like_help(argc, argv)) + return help(); + + umask(0022); + + return dispatch_verb(argc, argv, verbs, NULL); +} + DEFINE_MAIN_FUNCTION(run);