From b8c160cf281c6beda004c13bc6aaac1f6a73774d Mon Sep 17 00:00:00 2001 From: jash8506 Date: Mon, 14 Sep 2026 11:07:40 +1000 Subject: [PATCH] Skip device-scoped batteries and stop masking missing sysfs attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Peripherals with their own cell — wireless keyboards and mice, headsets, controllers — register in /sys/class/power_supply with type "Battery". ListBatteries returned them alongside the real system battery, so `set`, `apply` and `persist enable` tried to write charge thresholds to, say, a Logitech keyboard's hidpp_battery_0, which exposes no threshold attributes: Error on hidpp_battery_0: writing "40" to /sys/class/power_supply/ hidpp_battery_0/charge_control_start_threshold: permission denied `set` aborts on that error, so the thresholds it had already written to BAT0 were never saved to /etc/batctl.conf and silently reverted on the next boot. The failure is intermittent because these devices leave sysfs when the peripheral sleeps or is unplugged. The kernel already marks such supplies: it reports POWER_SUPPLY_SCOPE_DEVICE through the "scope" attribute (hid-logitech-hidpp hardcodes it). Skip anything device-scoped. Batteries without a "scope" attribute are still treated as system batteries, so laptop batteries — including those on platform backends such as IdeaPad, Huawei, Samsung, Sony and Acer, which drive thresholds through a platform device and expose no per-battery attributes — are unaffected. The error also lied about the cause. SysfsWriteString used os.WriteFile, which passes O_CREATE; sysfs refuses to create files and answers EACCES rather than ENOENT, so a missing attribute looked like a privilege problem even under sudo. Open without O_CREATE so the real "no such file or directory" surfaces. The two write tests wrote to paths that did not exist, relying on the O_CREATE that caused the bug. sysfs attributes always exist ahead of time, so they now pre-create the attribute, matching real behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- internal/battery/sysfs.go | 31 ++++++++++--- internal/battery/sysfs_test.go | 85 ++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 6 deletions(-) diff --git a/internal/battery/sysfs.go b/internal/battery/sysfs.go index 3c6ccc9..a1875fd 100644 --- a/internal/battery/sysfs.go +++ b/internal/battery/sysfs.go @@ -8,7 +8,7 @@ import ( "strings" ) -const PowerSupplyBase = "/sys/class/power_supply" +var PowerSupplyBase = "/sys/class/power_supply" func SysfsReadString(path string) (string, error) { data, err := os.ReadFile(path) @@ -35,7 +35,15 @@ func SysfsWriteInt(path string, value int) error { } func SysfsWriteString(path string, value string) error { - if err := os.WriteFile(path, []byte(value), 0644); err != nil { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, 0) + if err != nil { + return fmt.Errorf("writing %q to %s: %w", value, path, err) + } + if _, err := f.WriteString(value); err != nil { + f.Close() + return fmt.Errorf("writing %q to %s: %w", value, path, err) + } + if err := f.Close(); err != nil { return fmt.Errorf("writing %q to %s: %w", value, path, err) } return nil @@ -62,14 +70,25 @@ func ListBatteries() []string { } var bats []string for _, e := range entries { - typePath := filepath.Join(PowerSupplyBase, e.Name(), "type") - t, err := SysfsReadString(typePath) + t, err := SysfsReadString(filepath.Join(PowerSupplyBase, e.Name(), "type")) if err != nil { continue } - if strings.EqualFold(t, "battery") { - bats = append(bats, e.Name()) + if !strings.EqualFold(t, "battery") { + continue + } + if IsDeviceScoped(e.Name()) { + continue } + bats = append(bats, e.Name()) } return bats } + +func IsDeviceScoped(name string) bool { + scope, err := SysfsReadString(filepath.Join(PowerSupplyBase, name, "scope")) + if err != nil { + return false + } + return strings.EqualFold(scope, "device") +} diff --git a/internal/battery/sysfs_test.go b/internal/battery/sysfs_test.go index de4d018..9b3bfc5 100644 --- a/internal/battery/sysfs_test.go +++ b/internal/battery/sysfs_test.go @@ -1,6 +1,8 @@ package battery import ( + "errors" + "io/fs" "os" "path/filepath" "testing" @@ -107,6 +109,7 @@ func TestSysfsReadInt(t *testing.T) { func TestSysfsWriteInt(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "write_int") + os.WriteFile(path, []byte("0"), 0644) if err := SysfsWriteInt(path, 80); err != nil { t.Fatalf("unexpected error: %v", err) @@ -124,6 +127,7 @@ func TestSysfsWriteInt(t *testing.T) { func TestSysfsWriteString(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "write_str") + os.WriteFile(path, []byte("inhibit-charge"), 0644) if err := SysfsWriteString(path, "auto"); err != nil { t.Fatalf("unexpected error: %v", err) @@ -155,3 +159,84 @@ func TestSysfsExists(t *testing.T) { } }) } + +func TestSysfsWriteStringMissingAttribute(t *testing.T) { + path := filepath.Join(t.TempDir(), "charge_control_start_threshold") + + err := SysfsWriteString(path, "40") + if err == nil { + t.Fatal("expected an error writing to a missing attribute") + } + if !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("got %v, want a not-exist error", err) + } + if errors.Is(err, fs.ErrPermission) { + t.Fatalf("missing attribute reported as a permission error: %v", err) + } + if SysfsExists(path) { + t.Fatal("SysfsWriteString must not create the attribute") + } +} + +func writeFixture(t *testing.T, dir, name string, attrs map[string]string) { + t.Helper() + d := filepath.Join(dir, name) + if err := os.MkdirAll(d, 0755); err != nil { + t.Fatal(err) + } + for k, v := range attrs { + if err := os.WriteFile(filepath.Join(d, k), []byte(v+"\n"), 0644); err != nil { + t.Fatal(err) + } + } +} + +func TestListBatteries(t *testing.T) { + dir := t.TempDir() + writeFixture(t, dir, "AC", map[string]string{"type": "Mains"}) + writeFixture(t, dir, "BAT0", map[string]string{"type": "Battery"}) + writeFixture(t, dir, "BAT1", map[string]string{"type": "Battery", "scope": "System"}) + writeFixture(t, dir, "hidpp_battery_0", map[string]string{"type": "Battery", "scope": "Device"}) + writeFixture(t, dir, "ucsi-source-psy-USBC000:001", map[string]string{"type": "USB", "scope": "Device"}) + + orig := PowerSupplyBase + PowerSupplyBase = dir + defer func() { PowerSupplyBase = orig }() + + got := ListBatteries() + want := []string{"BAT0", "BAT1"} + if len(got) != len(want) { + t.Fatalf("ListBatteries() = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("ListBatteries() = %v, want %v", got, want) + } + } +} + +func TestIsDeviceScoped(t *testing.T) { + dir := t.TempDir() + writeFixture(t, dir, "BAT0", map[string]string{"type": "Battery"}) + writeFixture(t, dir, "hidpp_battery_0", map[string]string{"type": "Battery", "scope": "Device"}) + writeFixture(t, dir, "BAT1", map[string]string{"type": "Battery", "scope": "System"}) + + orig := PowerSupplyBase + PowerSupplyBase = dir + defer func() { PowerSupplyBase = orig }() + + tests := []struct { + name string + want bool + }{ + {"BAT0", false}, + {"BAT1", false}, + {"hidpp_battery_0", true}, + {"nonexistent", false}, + } + for _, tt := range tests { + if got := IsDeviceScoped(tt.name); got != tt.want { + t.Errorf("IsDeviceScoped(%q) = %v, want %v", tt.name, got, tt.want) + } + } +}