4c78479 fix(powerkey): keep the old lock until a new one is taken, correct keysym docs, wait on log events in tests · neferwl

M docs/desktop.md +10 -3


@@ -102,14 +102,21 @@ exec wlopm --off '*'
102	102
103	103		## Power keys
104	104
105		-	NeferWL takes logind's `handle-power-key`, `handle-suspend-key`, `handle-hibernate-key` and `handle-reboot-key` inhibitor locks. logind then no longer powers off, suspends, hibernates or reboots when one of those keys is pressed: the key reaches NeferWL as `XF86PowerOff`, `XF86Sleep` or `XF86Suspend`, `XF86Hibernate`, and nothing happens unless a bind uses it.
	105	+	NeferWL takes logind's `handle-power-key`, `handle-suspend-key`, `handle-hibernate-key` and `handle-reboot-key` inhibitor locks. logind then no longer powers off, suspends, hibernates or reboots when one of those keys is pressed: the key reaches NeferWL, and nothing happens unless a bind uses it.
	106	+
	107	+	\| logind key \| Kernel key \| Bind \|
	108	+	\|---\|---\|---\|
	109	+	\| power \| `KEY_POWER` \| `XF86PowerOff` \|
	110	+	\| suspend \| `KEY_SLEEP` \| `XF86Sleep` \|
	111	+	\| hibernate \| `KEY_SUSPEND` \| `XF86Suspend` \|
	112	+	\| reboot \| `KEY_RESTART` \| `code:408` (no keysym) \|
106	113
107	114		```text
108	115		bind.XF86PowerOff = spawn systemctl suspend
109		-	bind.XF86Hibernate = spawn systemctl hibernate
	116	+	bind.XF86Suspend = spawn systemctl hibernate
110	117		```
111	118
112		-	The lid switch is not covered: logind still handles it. The locks last as long as NeferWL runs; if it crashes, logind handles the keys again. If the system bus or logind is missing, NeferWL logs a warning and tries again later, and the keys keep logind's behaviour until it succeeds.
	119	+	The lid switch is not covered: logind still handles it. The locks last as long as NeferWL runs; if it crashes, logind handles the keys again. When logind restarts, NeferWL keeps the old locks until it has taken new ones. If the system bus or logind is missing, NeferWL logs a warning and tries again later, and the keys keep logind's behaviour until it succeeds.
113	120
114	121		## Bars
115	122

 ## Power keys
-NeferWL takes logind's `handle-power-key`, `handle-suspend-key`, `handle-hibernate-key` and `handle-reboot-key` inhibitor locks. logind then no longer powers off, suspends, hibernates or reboots when one of those keys is pressed: the key reaches NeferWL as `XF86PowerOff`, `XF86Sleep` or `XF86Suspend`, `XF86Hibernate`, and nothing happens unless a bind uses it.
+NeferWL takes logind's `handle-power-key`, `handle-suspend-key`, `handle-hibernate-key` and `handle-reboot-key` inhibitor locks. logind then no longer powers off, suspends, hibernates or reboots when one of those keys is pressed: the key reaches NeferWL, and nothing happens unless a bind uses it.
+| logind key | Kernel key | Bind |
+|---|---|---|
+| power | `KEY_POWER` | `XF86PowerOff` |
+| suspend | `KEY_SLEEP` | `XF86Sleep` |
+| hibernate | `KEY_SUSPEND` | `XF86Suspend` |
+| reboot | `KEY_RESTART` | `code:408` (no keysym) |
 ```text
 bind.XF86PowerOff = spawn systemctl suspend
-bind.XF86Hibernate = spawn systemctl hibernate
+bind.XF86Suspend = spawn systemctl hibernate
 ```
-The lid switch is not covered: logind still handles it. The locks last as long as NeferWL runs; if it crashes, logind handles the keys again. If the system bus or logind is missing, NeferWL logs a warning and tries again later, and the keys keep logind's behaviour until it succeeds.
+The lid switch is not covered: logind still handles it. The locks last as long as NeferWL runs; if it crashes, logind handles the keys again. When logind restarts, NeferWL keeps the old locks until it has taken new ones. If the system bus or logind is missing, NeferWL logs a warning and tries again later, and the keys keep logind's behaviour until it succeeds.
 ## Bars

M internal/adapters/powerkey/powerkey.go +22 -11


@@ -31,17 +31,25 @@ const (
31	31
32	32		// Serve holds the locks until ctx ends. address is the bus to use, the
33	33		// system bus when empty. When the bus or logind goes, it takes the locks
34		-	// again after retry, doubling up to busretry.Max: logind drops a lock with
35		-	// its own restart, and then handles the keys itself until we ask again.
	34	+	// again after retry, doubling up to a minute. Meanwhile the old lock stays
	35	+	// open: logind keeps an inhibitor across its own restart for as long as a
	36	+	// client holds the descriptor, so the keys stay blocked in the gap, and the
	37	+	// old lock is closed only once a new one is in place.
36	38		func Serve(ctx context.Context, address string, retry time.Duration, log zerowrap.Logger) {
	39	+	var lock *os.File
	40	+	defer func() {
	41	+	if lock != nil {
	42	+	_ = lock.Close()
	43	+	}
	44	+	}()
37	45		busretry.Run(ctx, retry, "logind power key lock", log, func(ctx context.Context) error {
38		-	return hold(ctx, address, log)
	46	+	return hold(ctx, address, log, &lock)
39	47		})
40	48		}
41	49
42		-	// hold takes the locks and keeps them until ctx ends (nil) or the bus or
43		-	// logind goes (an error).
44		-	func hold(ctx context.Context, address string, log zerowrap.Logger) error {
	50	+	// hold takes the locks, replaces *lock with them, and waits until ctx ends
	51	+	// (nil) or the bus or logind goes (an error).
	52	+	func hold(ctx context.Context, address string, log zerowrap.Logger, lock **os.File) error {
45	53		conn, err := connect(ctx, address)
46	54		if err != nil {
47	55		return err

@@ -60,9 +68,12 @@ func hold(ctx context.Context, address string, log zerowrap.Logger) error {
60	68		if err := conn.Object(logind, logindPath).CallWithContext(callCtx, manager+".Inhibit", 0, what, "neferwl", "power keys are bound in the compositor", "block").Store(&fd); err != nil {
61	69		return fmt.Errorf("inhibit %s: %w", what, err)
62	70		}
63		-	// logind releases the lock when this descriptor closes.
64		-	lock := os.NewFile(uintptr(fd), "logind-inhibit")
65		-	defer func() { _ = lock.Close() }()
	71	+	// logind releases a lock when its descriptor closes.
	72	+	old := *lock
	73	+	*lock = os.NewFile(uintptr(fd), "logind-inhibit")
	74	+	if old != nil {
	75	+	_ = old.Close()
	76	+	}
66	77		log.Info().Str("what", what).Msg("holding logind key locks")
67	78		for {
68	79		select {

@@ -91,14 +102,14 @@ func connect(ctx context.Context, address string) (*dbus.Conn, error) {
91	102		conn, err = dbus.Dial(address, dbus.WithContext(ctx))
92	103		}
93	104		if err != nil {
94		-	return nil, fmt.Errorf("system bus: %w", err)
	105	+	return nil, fmt.Errorf("bus: %w", err)
95	106		}
96	107		if err = conn.Auth(nil); err == nil {
97	108		err = conn.Hello()
98	109		}
99	110		if err != nil {
100	111		_ = conn.Close()
101		-	return nil, fmt.Errorf("system bus: %w", err)
	112	+	return nil, fmt.Errorf("bus: %w", err)
102	113		}
103	114		return conn, nil
104	115		}

 // Serve holds the locks until ctx ends. address is the bus to use, the
 // system bus when empty. When the bus or logind goes, it takes the locks
-// again after retry, doubling up to busretry.Max: logind drops a lock with
+// again after retry, doubling up to a minute. Meanwhile the old lock stays
-// its own restart, and then handles the keys itself until we ask again.
+// open: logind keeps an inhibitor across its own restart for as long as a
+// client holds the descriptor, so the keys stay blocked in the gap, and the
+// old lock is closed only once a new one is in place.
 func Serve(ctx context.Context, address string, retry time.Duration, log zerowrap.Logger) {
+	var lock *os.File
+	defer func() {
+		if lock != nil {
+			_ = lock.Close()
+		}
+	}()
 	busretry.Run(ctx, retry, "logind power key lock", log, func(ctx context.Context) error {
-		return hold(ctx, address, log)
+		return hold(ctx, address, log, &lock)
 	})
 }
-// hold takes the locks and keeps them until ctx ends (nil) or the bus or
+// hold takes the locks, replaces *lock with them, and waits until ctx ends
-// logind goes (an error).
+// (nil) or the bus or logind goes (an error).
-func hold(ctx context.Context, address string, log zerowrap.Logger) error {
+func hold(ctx context.Context, address string, log zerowrap.Logger, lock **os.File) error {
 	conn, err := connect(ctx, address)
 	if err != nil {
 		return err
 	if err := conn.Object(logind, logindPath).CallWithContext(callCtx, manager+".Inhibit", 0, what, "neferwl", "power keys are bound in the compositor", "block").Store(&fd); err != nil {
 		return fmt.Errorf("inhibit %s: %w", what, err)
 	}
-	// logind releases the lock when this descriptor closes.
+	// logind releases a lock when its descriptor closes.
-	lock := os.NewFile(uintptr(fd), "logind-inhibit")
+	old := *lock
-	defer func() { _ = lock.Close() }()
+	*lock = os.NewFile(uintptr(fd), "logind-inhibit")
+	if old != nil {
+		_ = old.Close()
+	}
 	log.Info().Str("what", what).Msg("holding logind key locks")
 	for {
 		select {
 		conn, err = dbus.Dial(address, dbus.WithContext(ctx))
 	}
 	if err != nil {
-		return nil, fmt.Errorf("system bus: %w", err)
+		return nil, fmt.Errorf("bus: %w", err)
 	}
 	if err = conn.Auth(nil); err == nil {
 		err = conn.Hello()
 	}
 	if err != nil {
 		_ = conn.Close()
-		return nil, fmt.Errorf("system bus: %w", err)
+		return nil, fmt.Errorf("bus: %w", err)
 	}
 	return conn, nil
 }

M internal/adapters/powerkey/powerkey_test.go +19 -8


@@ -120,17 +120,23 @@ func (l *logBuf) count(s string) int {
120	120		return strings.Count(l.buf.String(), s)
121	121		}
122	122
123		-	// holding waits until Serve has received n lock answers: only then does it
124		-	// own the descriptor.
125		-	func (l logBuf) holding(t testing.T, n int) {
	123	+	// wait blocks until the log has n lines containing msg.
	124	+	func (l logBuf) wait(t testing.T, msg string, n int) {
126	125		t.Helper()
127		-	for deadline := time.Now().Add(5 * time.Second); l.count("holding logind key locks") < n; time.Sleep(5 * time.Millisecond) {
	126	+	for deadline := time.Now().Add(5 * time.Second); l.count(msg) < n; time.Sleep(5 * time.Millisecond) {
128	127		if time.Now().After(deadline) {
129		-	t.Fatalf("Serve never held lock %d", n)
	128	+	t.Fatalf("log never had %d x %q", n, msg)
130	129		}
131	130		}
132	131		}
133	132
	133	+	// holding waits until Serve has received n lock answers: only then does it
	134	+	// own the descriptor.
	135	+	func (l logBuf) holding(t testing.T, n int) {
	136	+	t.Helper()
	137	+	l.wait(t, "holding logind key locks", n)
	138	+	}
	139	+
134	140		func run(t testing.T, address string) (stop func(), log logBuf) {
135	141		t.Helper()
136	142		ctx, cancel := context.WithCancel(context.Background())

@@ -184,19 +190,24 @@ func TestServeHoldsAndReleasesTheLock(t *testing.T) {
184	190		}
185	191		}
186	192
187		-	// When logind restarts, the locks are taken again.
	193	+	// When logind restarts, the locks are taken again and the old ones closed.
188	194		func TestServeTakesTheLockAgainAfterLogindRestarts(t *testing.T) {
189	195		address := startBus(t)
190	196		peer := newPeer(t, address)
191	197		_, log := run(t, address)
192		-	peer.next(t)
	198	+	first := peer.next(t)
193	199		log.holding(t, 1)
194	200		if _, err := peer.conn.ReleaseName(logind); err != nil {
195	201		t.Fatal(err)
196	202		}
	203	+	log.wait(t, "ignored until the next attempt", 1)
	204	+	// The gap: the old lock is still ours, logind keeps it through a restart.
197	205		peer.own(t)
198	206		peer.next(t)
199	207		log.holding(t, 2)
	208	+	if !released(t, first) {
	209	+	t.Fatal("the first lock was not closed once the second was taken")
	210	+	}
200	211		}
201	212
202	213		// Without logind on the bus, Serve keeps trying until it appears.

@@ -207,7 +218,7 @@ func TestServeWaitsForLogind(t *testing.T) {
207	218		t.Fatal(err)
208	219		}
209	220		_, log := run(t, address)
210		-	time.Sleep(50 * time.Millisecond)
	221	+	log.wait(t, "ignored until the next attempt", 1)
211	222		peer.own(t)
212	223		peer.next(t)
213	224		log.holding(t, 1)

 	return strings.Count(l.buf.String(), s)
 }
-// holding waits until Serve has received n lock answers: only then does it
+// wait blocks until the log has n lines containing msg.
-// own the descriptor.
+func (l *logBuf) wait(t *testing.T, msg string, n int) {
-func (l *logBuf) holding(t *testing.T, n int) {
 	t.Helper()
-	for deadline := time.Now().Add(5 * time.Second); l.count("holding logind key locks") < n; time.Sleep(5 * time.Millisecond) {
+	for deadline := time.Now().Add(5 * time.Second); l.count(msg) < n; time.Sleep(5 * time.Millisecond) {
 		if time.Now().After(deadline) {
-			t.Fatalf("Serve never held lock %d", n)
+			t.Fatalf("log never had %d x %q", n, msg)
 		}
 	}
 }
+// holding waits until Serve has received n lock answers: only then does it
+// own the descriptor.
+func (l *logBuf) holding(t *testing.T, n int) {
+	t.Helper()
+	l.wait(t, "holding logind key locks", n)
+}
 func run(t *testing.T, address string) (stop func(), log *logBuf) {
 	t.Helper()
 	ctx, cancel := context.WithCancel(context.Background())
 	}
 }
-// When logind restarts, the locks are taken again.
+// When logind restarts, the locks are taken again and the old ones closed.
 func TestServeTakesTheLockAgainAfterLogindRestarts(t *testing.T) {
 	address := startBus(t)
 	peer := newPeer(t, address)
 	_, log := run(t, address)
-	peer.next(t)
+	first := peer.next(t)
 	log.holding(t, 1)
 	if _, err := peer.conn.ReleaseName(logind); err != nil {
 		t.Fatal(err)
 	}
+	log.wait(t, "ignored until the next attempt", 1)
+	// The gap: the old lock is still ours, logind keeps it through a restart.
 	peer.own(t)
 	peer.next(t)
 	log.holding(t, 2)
+	if !released(t, first) {
+		t.Fatal("the first lock was not closed once the second was taken")
+	}
 }
 // Without logind on the bus, Serve keeps trying until it appears.
 		t.Fatal(err)
 	}
 	_, log := run(t, address)
-	time.Sleep(50 * time.Millisecond)
+	log.wait(t, "ignored until the next attempt", 1)
 	peer.own(t)
 	peer.next(t)
 	log.holding(t, 1)