3462c76 fix(config): read the config without blocking and tolerate invalid UTF-8 · neferwl

M docs/config.md +1 -1


@@ -4,7 +4,7 @@ NeferWL reads `$XDG_CONFIG_HOME/neferwl/config` (or `~/.config/neferwl/config`).
4	4
5	5		## Format
6	6
7		-	- One `key = value` per line; `#` starts a comment.
	7	+	- One `key = value` per line. A line starting with `#` is a comment, and so is a `#` after a space, unless it is inside a quoted word (`"a # b"`).
8	8		- Lists are comma-separated; booleans are `on` / `off`.
9	9		- A missing file or key means the default. An invalid line logs a warning and keeps that key's default; the rest of the file still applies. A duplicate key warns and the last one wins.
10	10		- Every key applies live when the file is saved; windows stay open. `startup` and `xwayland` take effect at the next start.


@@ -4,7 +4,7 @@ NeferWL reads `$XDG_CONFIG_HOME/neferwl/config` (or `~/.config/neferwl/config`).
4			4
5		## Format	5		## Format
6			6
7	-	- One `key = value` per line; `#` starts a comment.	7	+	- One `key = value` per line. A line starting with `#` is a comment, and so is a `#` after a space, unless it is inside a quoted word (`"a # b"`).
8		- Lists are comma-separated; booleans are `on` / `off`.	8		- Lists are comma-separated; booleans are `on` / `off`.
9		- A missing file or key means the default. An invalid line logs a warning and keeps that key's default; the rest of the file still applies. A duplicate key warns and the last one wins.	9		- A missing file or key means the default. An invalid line logs a warning and keeps that key's default; the rest of the file still applies. A duplicate key warns and the last one wins.
10		- Every key applies live when the file is saved; windows stay open. `startup` and `xwayland` take effect at the next start.	10		- Every key applies live when the file is saved; windows stay open. `startup` and `xwayland` take effect at the next start.

M internal/adapters/config/config.go +36 -11


@@ -2,9 +2,11 @@
2	2		package config
3	3
4	4		import (
	5	+	"bytes"
5	6		"errors"
6	7		"fmt"
7	8		"io"
	9	+	"io/fs"
8	10		"math"
9	11		"os"
10	12		"path/filepath"

@@ -15,6 +17,7 @@ import (
15	17		"strings"
16	18		"time"
17	19		"unicode"
	20	+	"unicode/utf8"
18	21
19	22		"github.com/bnema/kvconf"
20	23		"github.com/bnema/neferwl/internal/ports"

@@ -222,7 +225,7 @@ func Defaults() ports.Config {
222	225		// LoadDefault loads DefaultPath; a missing file means defaults.
223	226		func LoadDefault() (ports.Config, []Warning, error) {
224	227		c, w, err := Load(DefaultPath())
225		-	if os.IsNotExist(err) {
	228	+	if errors.Is(err, fs.ErrNotExist) {
226	229		return Defaults(), nil, nil
227	230		}
228	231		return c, w, err

@@ -230,12 +233,20 @@ func LoadDefault() (ports.Config, []Warning, error) {
230	233
231	234		// Load reads a config file. The error is only for I/O; bad lines become warnings.
232	235		func Load(path string) (ports.Config, []Warning, error) {
233		-	f, err := os.Open(path)
	236	+	c, _, w, err := loadFile(path)
234	237		if err != nil {
235	238		return Defaults(), nil, err
236	239		}
237		-	defer f.Close()
238		-	return Parse(f)
	240	+	return c, w, nil
	241	+	}
	242	+
	243	+	// loadFile reads path without blocking on a FIFO or device, then parses it.
	244	+	func loadFile(path string) (ports.Config, map[string]string, []Warning, error) {
	245	+	data, err := kvconf.ReadFile(path, kvconf.Options{MaxBytes: -1})
	246	+	if err != nil {
	247	+	return ports.Config{}, nil, nil, err
	248	+	}
	249	+	return parseBytes(data)
239	250		}
240	251
241	252		// maxOutputOffset bounds output.<name>.offset (logical px), far beyond any

@@ -317,12 +328,17 @@ func checkOutputAnchors(outputs []ports.OutputConfig, relKeys map[string]string,
317	328
318	329		// Parse reads `key = value` lines on top of the defaults.
319	330		func Parse(r io.Reader) (ports.Config, []Warning, error) {
320		-	c, _, w, err := parse(r)
	331	+	data, err := io.ReadAll(r)
	332	+	if err != nil {
	333	+	return Defaults(), nil, err
	334	+	}
	335	+	c, _, w, err := parseBytes(data)
321	336		return c, w, err
322	337		}
323	338
324		-	// parse also returns the effective key/value pairs, used to log what changed on reload.
325		-	func parse(r io.Reader) (ports.Config, map[string]string, []Warning, error) {
	339	+	// parseBytes also returns the effective key/value pairs, used to log what
	340	+	// changed on reload.
	341	+	func parseBytes(data []byte) (ports.Config, map[string]string, []Warning, error) {
326	342		c := Defaults()
327	343		var warnings []Warning
328	344		raw := map[string]string{}

@@ -333,12 +349,21 @@ func parse(r io.Reader) (ports.Config, map[string]string, []Warning, error) {
333	349		workspaces := map[string]int{}
334	350		rules := map[string]*ruleEntry{}
335	351		var ruleOrder []*ruleEntry
	352	+	// An invalid byte (a Latin-1 comment, say) costs a warning, not the
	353	+	// whole file: the rest still applies, like any other bad line.
	354	+	if !utf8.Valid(data) {
	355	+	line := 1
	356	+	for _, l := range bytes.Split(data, []byte("\n")) {
	357	+	if !utf8.Valid(l) {
	358	+	break
	359	+	}
	360	+	line++
	361	+	}
	362	+	data = bytes.ToValidUTF8(data, []byte("\uFFFD"))
	363	+	warnings = append(warnings, Warning{Line: line, Msg: "not valid UTF-8; invalid bytes replaced"})
	364	+	}
336	365		// kvconf owns the line syntax; the values stay raw (neferwl has no
337	366		// quoting) and repeated keys are reported below with their own wording.
338		-	data, err := io.ReadAll(r)
339		-	if err != nil {
340		-	return c, raw, nil, err
341		-	}
342	367		entries, lineWarnings, err := kvconf.ParseBytes(data, kvconf.Options{AllowDuplicates: true, RawValues: true, MaxBytes: -1, MaxLine: -1})
343	368		if err != nil {
344	369		return c, raw, nil, err

 package config
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"math"
 	"os"
 	"path/filepath"
 	"strings"
 	"time"
 	"unicode"
+	"unicode/utf8"
 	"github.com/bnema/kvconf"
 	"github.com/bnema/neferwl/internal/ports"
 // LoadDefault loads DefaultPath; a missing file means defaults.
 func LoadDefault() (ports.Config, []Warning, error) {
 	c, w, err := Load(DefaultPath())
-	if os.IsNotExist(err) {
+	if errors.Is(err, fs.ErrNotExist) {
 		return Defaults(), nil, nil
 	}
 	return c, w, err
 // Load reads a config file. The error is only for I/O; bad lines become warnings.
 func Load(path string) (ports.Config, []Warning, error) {
-	f, err := os.Open(path)
+	c, _, w, err := loadFile(path)
 	if err != nil {
 		return Defaults(), nil, err
 	}
-	defer f.Close()
+	return c, w, nil
-	return Parse(f)
+}
+// loadFile reads path without blocking on a FIFO or device, then parses it.
+func loadFile(path string) (ports.Config, map[string]string, []Warning, error) {
+	data, err := kvconf.ReadFile(path, kvconf.Options{MaxBytes: -1})
+	if err != nil {
+		return ports.Config{}, nil, nil, err
+	}
+	return parseBytes(data)
 }
 // maxOutputOffset bounds output.<name>.offset (logical px), far beyond any
 // Parse reads `key = value` lines on top of the defaults.
 func Parse(r io.Reader) (ports.Config, []Warning, error) {
-	c, _, w, err := parse(r)
+	data, err := io.ReadAll(r)
+	if err != nil {
+		return Defaults(), nil, err
+	}
+	c, _, w, err := parseBytes(data)
 	return c, w, err
 }
-// parse also returns the effective key/value pairs, used to log what changed on reload.
+// parseBytes also returns the effective key/value pairs, used to log what
-func parse(r io.Reader) (ports.Config, map[string]string, []Warning, error) {
+// changed on reload.
+func parseBytes(data []byte) (ports.Config, map[string]string, []Warning, error) {
 	c := Defaults()
 	var warnings []Warning
 	raw := map[string]string{}
 	workspaces := map[string]int{}
 	rules := map[string]*ruleEntry{}
 	var ruleOrder []*ruleEntry
+	// An invalid byte (a Latin-1 comment, say) costs a warning, not the
+	// whole file: the rest still applies, like any other bad line.
+	if !utf8.Valid(data) {
+		line := 1
+		for _, l := range bytes.Split(data, []byte("\n")) {
+			if !utf8.Valid(l) {
+				break
+			}
+			line++
+		}
+		data = bytes.ToValidUTF8(data, []byte("\uFFFD"))
+		warnings = append(warnings, Warning{Line: line, Msg: "not valid UTF-8; invalid bytes replaced"})
+	}
 	// kvconf owns the line syntax; the values stay raw (neferwl has no
 	// quoting) and repeated keys are reported below with their own wording.
-	data, err := io.ReadAll(r)
-	if err != nil {
-		return c, raw, nil, err
-	}
 	entries, lineWarnings, err := kvconf.ParseBytes(data, kvconf.Options{AllowDuplicates: true, RawValues: true, MaxBytes: -1, MaxLine: -1})
 	if err != nil {
 		return c, raw, nil, err

M internal/adapters/config/config_test.go +69 -0


@@ -7,6 +7,7 @@ import (
7	7		"path/filepath"
8	8		"reflect"
9	9		"strings"
	10	+	"syscall"
10	11		"testing"
11	12		"time"
12	13

@@ -831,3 +832,71 @@ func TestOutputModeLineKeepsAnchor(t *testing.T) {
831	832		t.Fatalf("%+v", o)
832	833		}
833	834		}
	835	+
	836	+	func TestFileSyntax(t *testing.T) {
	837	+	for _, tc := range []struct {
	838	+	name, in string
	839	+	check func(t *testing.T, c ports.Config)
	840	+	warnings []string
	841	+	}{
	842	+	{
	843	+	name: "invalid UTF-8 costs a warning, not the file",
	844	+	in: "# caf\xe9\nborder.width = 5\n",
	845	+	check: func(t *testing.T, c ports.Config) {
	846	+	if c.Border.Width != 5 {
	847	+	t.Fatalf("border.width = %d", c.Border.Width)
	848	+	}
	849	+	},
	850	+	warnings: []string{"line 1: not valid UTF-8; invalid bytes replaced"},
	851	+	},
	852	+	{
	853	+	name: "output name with spaces",
	854	+	in: "output.LG Electronics 27GR95UM.scale = 1.5\n",
	855	+	check: func(t *testing.T, c ports.Config) {
	856	+	if len(c.Outputs) != 1 \|\| c.Outputs[0].Name != "LG Electronics 27GR95UM" \|\| c.Outputs[0].Scale != 1.5 {
	857	+	t.Fatalf("%+v", c.Outputs)
	858	+	}
	859	+	},
	860	+	},
	861	+	{
	862	+	// A '#' inside a quoted word is kept, as a shell would.
	863	+	name: "comment after a quoted word",
	864	+	in: "startup = notify-send \"a # b\" # note\n",
	865	+	check: func(t *testing.T, c ports.Config) {
	866	+	if !reflect.DeepEqual(c.Startup, [][]string{{"notify-send", `"a`, "#", `b"`}}) {
	867	+	t.Fatalf("startup: %q", c.Startup)
	868	+	}
	869	+	},
	870	+	},
	871	+	} {
	872	+	t.Run(tc.name, func(t *testing.T) {
	873	+	c, w := parseString(t, tc.in)
	874	+	got := make([]string, 0, len(w))
	875	+	for _, x := range w {
	876	+	got = append(got, x.String())
	877	+	}
	878	+	if len(got) != len(tc.warnings) \|\| (len(got) > 0 && !reflect.DeepEqual(got, tc.warnings)) {
	879	+	t.Fatalf("warnings %q, want %q", got, tc.warnings)
	880	+	}
	881	+	tc.check(t, c)
	882	+	})
	883	+	}
	884	+	}
	885	+
	886	+	// A FIFO in place of the config fails at once instead of blocking startup.
	887	+	func TestLoadRejectsFIFO(t *testing.T) {
	888	+	path := filepath.Join(t.TempDir(), "config")
	889	+	if err := syscall.Mkfifo(path, 0o600); err != nil {
	890	+	t.Skip(err)
	891	+	}
	892	+	done := make(chan error, 1)
	893	+	go func() { _, _, err := Load(path); done <- err }()
	894	+	select {
	895	+	case err := <-done:
	896	+	if err == nil {
	897	+	t.Fatal("FIFO loaded")
	898	+	}
	899	+	case <-time.After(5 * time.Second):
	900	+	t.Fatal("Load blocked on a FIFO")
	901	+	}
	902	+	}


@@ -7,6 +7,7 @@ import (
7	"path/filepath"	7		"path/filepath"
8	"reflect"	8		"reflect"
9	"strings"	9		"strings"
		10	+	"syscall"
10	"testing"	11		"testing"
11	"time"	12		"time"
12		13

@@ -831,3 +832,71 @@ func TestOutputModeLineKeepsAnchor(t *testing.T) {
831	t.Fatalf("%+v", o)	832		t.Fatalf("%+v", o)
832	}	833		}
833	}	834		}
		835	+
		836	+	func TestFileSyntax(t *testing.T) {
		837	+	for _, tc := range []struct {
		838	+	name, in string
		839	+	check func(t *testing.T, c ports.Config)
		840	+	warnings []string
		841	+	}{
		842	+	{
		843	+	name: "invalid UTF-8 costs a warning, not the file",
		844	+	in: "# caf\xe9\nborder.width = 5\n",
		845	+	check: func(t *testing.T, c ports.Config) {
		846	+	if c.Border.Width != 5 {
		847	+	t.Fatalf("border.width = %d", c.Border.Width)
		848	+	}
		849	+	},
		850	+	warnings: []string{"line 1: not valid UTF-8; invalid bytes replaced"},
		851	+	},
		852	+	{
		853	+	name: "output name with spaces",
		854	+	in: "output.LG Electronics 27GR95UM.scale = 1.5\n",
		855	+	check: func(t *testing.T, c ports.Config) {
		856	+	if len(c.Outputs) != 1 \|\| c.Outputs[0].Name != "LG Electronics 27GR95UM" \|\| c.Outputs[0].Scale != 1.5 {
		857	+	t.Fatalf("%+v", c.Outputs)
		858	+	}
		859	+	},
		860	+	},
		861	+	{
		862	+	// A '#' inside a quoted word is kept, as a shell would.
		863	+	name: "comment after a quoted word",
		864	+	in: "startup = notify-send \"a # b\" # note\n",
		865	+	check: func(t *testing.T, c ports.Config) {
		866	+	if !reflect.DeepEqual(c.Startup, [][]string{{"notify-send", `"a`, "#", `b"`}}) {
		867	+	t.Fatalf("startup: %q", c.Startup)
		868	+	}
		869	+	},
		870	+	},
		871	+	} {
		872	+	t.Run(tc.name, func(t *testing.T) {
		873	+	c, w := parseString(t, tc.in)
		874	+	got := make([]string, 0, len(w))
		875	+	for _, x := range w {
		876	+	got = append(got, x.String())
		877	+	}
		878	+	if len(got) != len(tc.warnings) \|\| (len(got) > 0 && !reflect.DeepEqual(got, tc.warnings)) {
		879	+	t.Fatalf("warnings %q, want %q", got, tc.warnings)
		880	+	}
		881	+	tc.check(t, c)
		882	+	})
		883	+	}
		884	+	}
		885	+
		886	+	// A FIFO in place of the config fails at once instead of blocking startup.
		887	+	func TestLoadRejectsFIFO(t *testing.T) {
		888	+	path := filepath.Join(t.TempDir(), "config")
		889	+	if err := syscall.Mkfifo(path, 0o600); err != nil {
		890	+	t.Skip(err)
		891	+	}
		892	+	done := make(chan error, 1)
		893	+	go func() { _, _, err := Load(path); done <- err }()
		894	+	select {
		895	+	case err := <-done:
		896	+	if err == nil {
		897	+	t.Fatal("FIFO loaded")
		898	+	}
		899	+	case <-time.After(5 * time.Second):
		900	+	t.Fatal("Load blocked on a FIFO")
		901	+	}
		902	+	}

M internal/adapters/config/rules_test.go +1 -1


@@ -116,7 +116,7 @@ func TestRuleReloadDiff(t *testing.T) {
116	116
117	117		func parseRaw(t *testing.T, s string) (cfg any, raw map[string]string, w []Warning) {
118	118		t.Helper()
119		-	c, raw, w, err := parse(strings.NewReader(s))
	119	+	c, raw, w, err := parseBytes([]byte(s))
120	120		if err != nil {
121	121		t.Fatal(err)
122	122		}


@@ -116,7 +116,7 @@ func TestRuleReloadDiff(t *testing.T) {
116			116
117		func parseRaw(t *testing.T, s string) (cfg any, raw map[string]string, w []Warning) {	117		func parseRaw(t *testing.T, s string) (cfg any, raw map[string]string, w []Warning) {
118		t.Helper()	118		t.Helper()
119	-	c, raw, w, err := parse(strings.NewReader(s))	119	+	c, raw, w, err := parseBytes([]byte(s))
120		if err != nil {	120		if err != nil {
121		t.Fatal(err)	121		t.Fatal(err)
122		}	122		}

M internal/adapters/config/scalestore_test.go +14 -0


@@ -117,3 +117,17 @@ func TestFormatScaleRoundTrip(t *testing.T) {
117	117		}
118	118		}
119	119		}
	120	+
	121	+	// Output names may hold spaces (EDID model names).
	122	+	func TestSaveOutputScaleSpacedName(t *testing.T) {
	123	+	path := filepath.Join(t.TempDir(), "config")
	124	+	if err := os.WriteFile(path, []byte("output.LG Electronics 27GR95UM.scale = 2\n"), 0o600); err != nil {
	125	+	t.Fatal(err)
	126	+	}
	127	+	if err := (ScaleStore{Path: path}).SaveOutputScale("LG Electronics 27GR95UM", 1.5); err != nil {
	128	+	t.Fatal(err)
	129	+	}
	130	+	if got, _ := os.ReadFile(path); string(got) != "output.LG Electronics 27GR95UM.scale = 1.5\n" {
	131	+	t.Fatalf("%q", got)
	132	+	}
	133	+	}


@@ -117,3 +117,17 @@ func TestFormatScaleRoundTrip(t *testing.T) {
117	}	117		}
118	}	118		}
119	}	119		}
		120	+
		121	+	// Output names may hold spaces (EDID model names).
		122	+	func TestSaveOutputScaleSpacedName(t *testing.T) {
		123	+	path := filepath.Join(t.TempDir(), "config")
		124	+	if err := os.WriteFile(path, []byte("output.LG Electronics 27GR95UM.scale = 2\n"), 0o600); err != nil {
		125	+	t.Fatal(err)
		126	+	}
		127	+	if err := (ScaleStore{Path: path}).SaveOutputScale("LG Electronics 27GR95UM", 1.5); err != nil {
		128	+	t.Fatal(err)
		129	+	}
		130	+	if got, _ := os.ReadFile(path); string(got) != "output.LG Electronics 27GR95UM.scale = 1.5\n" {
		131	+	t.Fatalf("%q", got)
		132	+	}
		133	+	}

M internal/adapters/config/watch.go +3 -13


@@ -2,7 +2,8 @@ package config
2	2
3	3		import (
4	4		"context"
5		-	"os"
	5	+	"errors"
	6	+	"io/fs"
6	7		"time"
7	8
8	9		"github.com/bnema/kvconf"

@@ -14,15 +15,6 @@ import (
14	15		// every write restarts it, so the file is never read half written.
15	16		const reloadDebounce = 100 * time.Millisecond
16	17
17		-	func loadFile(path string) (ports.Config, map[string]string, []Warning, error) {
18		-	f, err := os.Open(path)
19		-	if err != nil {
20		-	return ports.Config{}, nil, nil, err
21		-	}
22		-	defer f.Close()
23		-	return parse(f)
24		-	}
25		-
26	18		// loadRaw returns the effective keys of the file at startup; missing means none.
27	19		func loadRaw(path string) map[string]string {
28	20		_, raw, _, err := loadFile(path)

@@ -49,7 +41,6 @@ func Watch(ctx context.Context, path string, out chan<- ports.ConfigChanged, log
49	41		for {
50	42		select {
51	43		case <-ctx.Done():
52		-	cancel()
53	44		return <-watchErr
54	45		case err := <-watchErr:
55	46		return err

@@ -57,7 +48,7 @@ func Watch(ctx context.Context, path string, out chan<- ports.ConfigChanged, log
57	48		}
58	49		cfg, raw, warnings, err := loadFile(path)
59	50		switch {
60		-	case os.IsNotExist(err):
	51	+	case errors.Is(err, fs.ErrNotExist):
61	52		log.Warn().Msg("config file removed; keeping current config")
62	53		continue
63	54		case err != nil:

@@ -75,7 +66,6 @@ func Watch(ctx context.Context, path string, out chan<- ports.ConfigChanged, log
75	66		}
76	67		select {
77	68		case <-ctx.Done():
78		-	cancel()
79	69		return <-watchErr
80	70		case out <- ports.ConfigChanged{Config: cfg}:
81	71		}

 import (
 	"context"
-	"os"
+	"errors"
+	"io/fs"
 	"time"
 	"github.com/bnema/kvconf"
 // every write restarts it, so the file is never read half written.
 const reloadDebounce = 100 * time.Millisecond
-func loadFile(path string) (ports.Config, map[string]string, []Warning, error) {
-	f, err := os.Open(path)
-	if err != nil {
-		return ports.Config{}, nil, nil, err
-	}
-	defer f.Close()
-	return parse(f)
-}
 // loadRaw returns the effective keys of the file at startup; missing means none.
 func loadRaw(path string) map[string]string {
 	_, raw, _, err := loadFile(path)
 	for {
 		select {
 		case <-ctx.Done():
-			cancel()
 			return <-watchErr
 		case err := <-watchErr:
 			return err
 		}
 		cfg, raw, warnings, err := loadFile(path)
 		switch {
-		case os.IsNotExist(err):
+		case errors.Is(err, fs.ErrNotExist):
 			log.Warn().Msg("config file removed; keeping current config")
 			continue
 		case err != nil:
 		}
 		select {
 		case <-ctx.Done():
-			cancel()
 			return <-watchErr
 		case out <- ports.ConfigChanged{Config: cfg}:
 		}

M internal/adapters/config/watch_test.go +1 -1


@@ -220,7 +220,7 @@ func TestWatchSymlink(t *testing.T) {
220	220		t.Fatal(err)
221	221		}
222	222		write(moved, "background = #000001\n")
223		-	time.Sleep(100 * time.Millisecond) // past the 20 ms retry
	223	+	time.Sleep(100 * time.Millisecond) // let the watch move back into the recreated directory
224	224		until("#abcdef", func(body string) { write(moved, body) })
225	225		// The old target no longer matters.
226	226		write(target, "background = #123456\n")


@@ -220,7 +220,7 @@ func TestWatchSymlink(t *testing.T) {
220		t.Fatal(err)	220		t.Fatal(err)
221		}	221		}
222		write(moved, "background = #000001\n")	222		write(moved, "background = #000001\n")
223	-	time.Sleep(100 * time.Millisecond) // past the 20 ms retry	223	+	time.Sleep(100 * time.Millisecond) // let the watch move back into the recreated directory
224		until("#abcdef", func(body string) { write(moved, body) })	224		until("#abcdef", func(body string) { write(moved, body) })
225		// The old target no longer matters.	225		// The old target no longer matters.
226		write(target, "background = #123456\n")	226		write(target, "background = #123456\n")