system: wrap op errors

This passes more information allowing for better error handling. This eliminates generic WrapErr from system.

Signed-off-by: Ophestra <cat@gensokyo.uk>

system/acl.go

@@ -36,8 +36,7 @@ func (a *ACL) Type() Enablement { return a.et }
 
 func (a *ACL) apply(sys *I) error {
 	msg.Verbose("applying ACL", a)
-	return wrapErrSuffix(acl.Update(a.path, sys.uid, a.perms...),
+	return newOpError("acl", acl.Update(a.path, sys.uid, a.perms...), false)
-		fmt.Sprintf("cannot apply ACL entry to %q:", a.path))
 }
 
 func (a *ACL) revert(sys *I, ec *Criteria) error {
@@ -49,8 +48,7 @@ func (a *ACL) revert(sys *I, ec *Criteria) error {
 			msg.Verbosef("target of ACL %s no longer exists", a)
 			err = nil
 		}
-		return wrapErrSuffix(err,
+		return newOpError("acl", err, true)
-			fmt.Sprintf("cannot strip ACL entry from %q:", a.path))
 	} else {
 		msg.Verbose("skipping ACL", a)
 		return nil

system/dbus.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"fmt"
 	"log"
 	"strings"
 	"sync"
@@ -29,8 +30,8 @@ func (sys *I) ProxyDBus(session, system *dbus.Config, sessionPath, systemPath st
 
 	// session bus is required as otherwise this is effectively a very expensive noop
 	if session == nil {
-		return nil, msg.WrapErr(ErrDBusConfig,
+		return nil, newOpErrorMessage("dbus", ErrDBusConfig,
-			"attempted to create message bus proxy args without session bus config")
+			"attempted to create message bus proxy args without session bus config", false)
 	}
 
 	// system bus is optional
@@ -41,9 +42,11 @@ func (sys *I) ProxyDBus(session, system *dbus.Config, sessionPath, systemPath st
 	d.out = &scanToFmsg{msg: new(strings.Builder)}
 	if final, err := dbus.Finalise(d.sessionBus, d.systemBus, session, system); err != nil {
 		if errors.Is(err, syscall.EINVAL) {
-			return nil, msg.WrapErr(err, "message bus proxy configuration contains NUL byte")
+			return nil, newOpErrorMessage("dbus", err,
+				"message bus proxy configuration contains NUL byte", false)
 		}
-		return nil, wrapErrSuffix(err, "cannot finalise message bus proxy:")
+		return nil, newOpErrorMessage("dbus", err,
+			fmt.Sprintf("cannot finalise message bus proxy: %v", err), false)
 	} else {
 		if msg.IsVerbose() {
 			msg.Verbose("session bus proxy:", session.Args(d.sessionBus))
@@ -84,8 +87,8 @@ func (d *DBus) apply(sys *I) error {
 	d.proxy = dbus.New(sys.ctx, d.final, d.out)
 	if err := d.proxy.Start(); err != nil {
 		d.out.Dump()
-		return wrapErrSuffix(err,
+		return newOpErrorMessage("dbus", err,
-			"cannot start message bus proxy:")
+			fmt.Sprintf("cannot start message bus proxy: %v", err), false)
 	}
 	msg.Verbose("starting message bus proxy", d.proxy)
 	return nil
@@ -101,7 +104,8 @@ func (d *DBus) revert(*I, *Criteria) error {
 		msg.Verbose("message bus proxy canceled upstream")
 		err = nil
 	}
-	return wrapErrSuffix(err, "message bus proxy error:")
+	return newOpErrorMessage("dbus", err,
+		fmt.Sprintf("message bus proxy error: %v", err), true)
 }
 
 func (d *DBus) Is(o Op) bool {
@@ -111,13 +115,8 @@ func (d *DBus) Is(o Op) bool {
 			(d.proxy != nil && d0.proxy != nil && d.proxy.String() == d0.proxy.String()))
 }
 
-func (d *DBus) Path() string {
+func (d *DBus) Path() string   { return "(dbus proxy)" }
-	return "(dbus proxy)"
+func (d *DBus) String() string { return d.proxy.String() }
-}
-
-func (d *DBus) String() string {
-	return d.proxy.String()
-}
 
 type scanToFmsg struct {
 	msg    *strings.Builder
@@ -154,8 +153,8 @@ func (s *scanToFmsg) write(p []byte, a int) (int, error) {
 
 func (s *scanToFmsg) Dump() {
 	s.mu.RLock()
-	for _, msg := range s.msgbuf {
+	for _, m := range s.msgbuf {
-		log.Println("(dbus) " + msg)
+		log.Println("(dbus) " + m)
 	}
 	s.mu.RUnlock()
 }

system/link.go

@@ -27,15 +27,13 @@ func (l *Hardlink) Type() Enablement { return l.et }
 
 func (l *Hardlink) apply(*I) error {
 	msg.Verbose("linking", l)
-	return wrapErrSuffix(os.Link(l.src, l.dst),
+	return newOpError("hardlink", os.Link(l.src, l.dst), false)
-		fmt.Sprintf("cannot link %q:", l.dst))
 }
 
 func (l *Hardlink) revert(_ *I, ec *Criteria) error {
 	if ec.hasType(l) {
 		msg.Verbosef("removing hard link %q", l.dst)
-		return wrapErrSuffix(os.Remove(l.dst),
+		return newOpError("hardlink", os.Remove(l.dst), true)
-			fmt.Sprintf("cannot remove hard link %q:", l.dst))
 	} else {
 		msg.Verbosef("skipping hard link %q", l.dst)
 		return nil

system/mkdir.go

@@ -41,15 +41,15 @@ func (m *Mkdir) apply(*I) error {
 	msg.Verbose("ensuring directory", m)
 
 	// create directory
-	err := os.Mkdir(m.path, m.perm)
+	if err := os.Mkdir(m.path, m.perm); err != nil {
-	if !errors.Is(err, os.ErrExist) {
+		if !errors.Is(err, os.ErrExist) {
-		return wrapErrSuffix(err,
+			return newOpError("mkdir", err, false)
-			fmt.Sprintf("cannot create directory %q:", m.path))
+		}
+		// directory exists, ensure mode
+		return newOpError("mkdir", os.Chmod(m.path, m.perm), false)
+	} else {
+		return nil
 	}
-
-	// directory exists, ensure mode
-	return wrapErrSuffix(os.Chmod(m.path, m.perm),
-		fmt.Sprintf("cannot change mode of %q to %s:", m.path, m.perm))
 }
 
 func (m *Mkdir) revert(_ *I, ec *Criteria) error {
@@ -60,8 +60,7 @@ func (m *Mkdir) revert(_ *I, ec *Criteria) error {
 
 	if ec.hasType(m) {
 		msg.Verbose("destroying ephemeral directory", m)
-		return wrapErrSuffix(os.Remove(m.path),
+		return newOpError("mkdir", os.Remove(m.path), true)
-			fmt.Sprintf("cannot remove ephemeral directory %q:", m.path))
 	} else {
 		msg.Verbose("skipping ephemeral directory", m)
 		return nil

system/op.go

@@ -123,7 +123,7 @@ func (sys *I) Commit(ctx context.Context) error {
 			// rollback partial commit
 			msg.Verbosef("commit faulted after %d ops, rolling back partial commit", len(sp.ops))
 			if err := sp.Revert(nil); err != nil {
-				log.Println("errors returned reverting partial commit:", err)
+				printJoinedError(log.Println, "cannot revert partial commit:", err)
 			}
 		}
 	}()

system/op_internal_test.go

@@ -1,68 +0,0 @@
-package system
-
-import "testing"
-
-type tcOp struct {
-	et   Enablement
-	path string
-}
-
-// test an instance of the Op interface
-func (ptc tcOp) test(t *testing.T, gotOps []Op, wantOps []Op, fn string) {
-	if len(gotOps) != len(wantOps) {
-		t.Errorf("%s: inserted %v Ops, want %v", fn,
-			len(gotOps), len(wantOps))
-		return
-	}
-
-	t.Run("path", func(t *testing.T) {
-		if len(gotOps) > 0 {
-			if got := gotOps[0].Path(); got != ptc.path {
-				t.Errorf("Path() = %q, want %q",
-					got, ptc.path)
-				return
-			}
-		}
-	})
-
-	for i := range gotOps {
-		o := gotOps[i]
-
-		t.Run("is", func(t *testing.T) {
-			if !o.Is(o) {
-				t.Errorf("Is returned false on self")
-				return
-			}
-			if !o.Is(wantOps[i]) {
-				t.Errorf("%s: inserted %#v, want %#v",
-					fn,
-					o, wantOps[i])
-				return
-			}
-		})
-
-		t.Run("criteria", func(t *testing.T) {
-			testCases := []struct {
-				name string
-				ec   *Criteria
-				want bool
-			}{
-				{"nil", nil, ptc.et != User},
-				{"self", newCriteria(ptc.et), true},
-				{"all", newCriteria(EWayland | EX11 | EDBus | EPulse | User | Process), true},
-				{"enablements", newCriteria(EWayland | EX11 | EDBus | EPulse), ptc.et != User && ptc.et != Process},
-			}
-
-			for _, tc := range testCases {
-				t.Run(tc.name, func(t *testing.T) {
-					if got := tc.ec.hasType(o); got != tc.want {
-						t.Errorf("hasType: got %v, want %v",
-							got, tc.want)
-					}
-				})
-			}
-		})
-	}
-}
-
-func newCriteria(e Enablement) *Criteria { return (*Criteria)(&e) }

system/output.go

@@ -1,6 +1,10 @@
 package system
 
 import (
+	"errors"
+	"net"
+	"os"
+
 	"hakurei.app/container"
 )
 
@@ -14,9 +18,59 @@ func SetOutput(v container.Msg) {
 	}
 }
 
-func wrapErrSuffix(err error, a ...any) error {
+// OpError is returned by [I.Commit] and [I.Revert].
+type OpError struct {
+	Op      string
+	Err     error
+	Message string
+	Revert  bool
+}
+
+func (e *OpError) Unwrap() error { return e.Err }
+func (e *OpError) Error() string {
+	if e.Message != "" {
+		return e.Message
+	}
+
+	switch {
+	case errors.As(e.Err, new(*os.PathError)), errors.As(e.Err, new(*net.OpError)):
+		return e.Err.Error()
+
+	default:
+		if !e.Revert {
+			return "cannot apply " + e.Op + ": " + e.Err.Error()
+		} else {
+			return "cannot revert " + e.Op + ": " + e.Err.Error()
+		}
+	}
+}
+
+// newOpError returns an [OpError] without a message string.
+func newOpError(op string, err error, revert bool) error {
 	if err == nil {
 		return nil
 	}
-	return msg.WrapErr(err, append(a, err)...)
+	return &OpError{op, err, "", revert}
+}
+
+// newOpErrorMessage returns an [OpError] with an overriding message string.
+func newOpErrorMessage(op string, err error, message string, revert bool) error {
+	if err == nil {
+		return nil
+	}
+	return &OpError{op, err, message, revert}
+}
+
+func printJoinedError(println func(v ...any), fallback string, err error) {
+	var joinErr interface {
+		Unwrap() []error
+		error
+	}
+	if !errors.As(err, &joinErr) {
+		println(fallback, err)
+	} else {
+		for _, err = range joinErr.Unwrap() {
+			println(err.Error())
+		}
+	}
 }

system/output_test.go

@@ -0,0 +1,189 @@
+package system
+
+import (
+	"errors"
+	"net"
+	"os"
+	"reflect"
+	"syscall"
+	"testing"
+
+	"hakurei.app/container"
+	"hakurei.app/internal/hlog"
+)
+
+func TestOpError(t *testing.T) {
+	testCases := []struct {
+		name string
+		err  error
+		s    string
+		is   error
+		isF  error
+	}{
+		{"message", newOpErrorMessage("dbus", ErrDBusConfig,
+			"attempted to create message bus proxy args without session bus config", false),
+			"attempted to create message bus proxy args without session bus config",
+			ErrDBusConfig, syscall.ENOTRECOVERABLE},
+
+		{"apply", newOpError("tmpfile", syscall.EBADE, false),
+			"cannot apply tmpfile: invalid exchange",
+			syscall.EBADE, syscall.EBADF},
+
+		{"revert", newOpError("wayland", syscall.EBADF, true),
+			"cannot revert wayland: bad file descriptor",
+			syscall.EBADF, syscall.EBADE},
+
+		{"path", newOpError("tmpfile", &os.PathError{Op: "stat", Path: "/run/dbus", Err: syscall.EISDIR}, false),
+			"stat /run/dbus: is a directory",
+			syscall.EISDIR, syscall.ENOTDIR},
+
+		{"net", newOpError("wayland", &net.OpError{Op: "dial", Net: "unix", Addr: &net.UnixAddr{Name: "/run/user/1000/wayland-1", Net: "unix"}, Err: syscall.ENOENT}, false),
+			"dial unix /run/user/1000/wayland-1: no such file or directory",
+			syscall.ENOENT, syscall.EPERM},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Run("error", func(t *testing.T) {
+				if got := tc.err.Error(); got != tc.s {
+					t.Errorf("Error: %q, want %q", got, tc.s)
+				}
+			})
+			t.Run("is", func(t *testing.T) {
+				if !errors.Is(tc.err, tc.is) {
+					t.Error("Is: unexpected false")
+				}
+				if errors.Is(tc.err, tc.isF) {
+					t.Error("Is: unexpected true")
+				}
+			})
+		})
+	}
+
+	t.Run("new", func(t *testing.T) {
+		if err := newOpError("check", nil, false); err != nil {
+			t.Errorf("newOpError: %v", err)
+		}
+		if err := newOpErrorMessage("check", nil, "", false); err != nil {
+			t.Errorf("newOpErrorMessage: %v", err)
+		}
+	})
+}
+
+func TestSetOutput(t *testing.T) {
+	oldmsg := msg
+	t.Cleanup(func() { msg = oldmsg })
+	msg = nil
+
+	t.Run("nil", func(t *testing.T) {
+		SetOutput(nil)
+		if _, ok := msg.(*container.DefaultMsg); !ok {
+			t.Errorf("SetOutput: %#v", msg)
+		}
+	})
+
+	t.Run("hlog", func(t *testing.T) {
+		SetOutput(hlog.Output{})
+		if _, ok := msg.(hlog.Output); !ok {
+			t.Errorf("SetOutput: %#v", msg)
+		}
+	})
+
+	t.Run("reset", func(t *testing.T) {
+		SetOutput(nil)
+		if _, ok := msg.(*container.DefaultMsg); !ok {
+			t.Errorf("SetOutput: %#v", msg)
+		}
+	})
+}
+
+func TestPrintJoinedError(t *testing.T) {
+	testCases := []struct {
+		name string
+		err  error
+		want [][]any
+	}{
+		{"nil", nil, [][]any{{"not a joined error:", nil}}},
+		{"unwrapped", syscall.EINVAL, [][]any{{"not a joined error:", syscall.EINVAL}}},
+		{"single", errors.Join(syscall.EINVAL), [][]any{{"invalid argument"}}},
+
+		{"many", errors.Join(syscall.ENOTRECOVERABLE, syscall.ETIMEDOUT, syscall.EBADFD), [][]any{
+			{"state not recoverable"},
+			{"connection timed out"},
+			{"file descriptor in bad state"},
+		}},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			var got [][]any
+			printJoinedError(func(v ...any) { got = append(got, v) }, "not a joined error:", tc.err)
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("printJoinedError: %#v, want %#v", got, tc.want)
+			}
+		})
+	}
+}
+
+type tcOp struct {
+	et   Enablement
+	path string
+}
+
+// test an instance of the Op interface
+func (ptc tcOp) test(t *testing.T, gotOps []Op, wantOps []Op, fn string) {
+	if len(gotOps) != len(wantOps) {
+		t.Errorf("%s: inserted %v Ops, want %v", fn,
+			len(gotOps), len(wantOps))
+		return
+	}
+
+	t.Run("path", func(t *testing.T) {
+		if len(gotOps) > 0 {
+			if got := gotOps[0].Path(); got != ptc.path {
+				t.Errorf("Path() = %q, want %q",
+					got, ptc.path)
+				return
+			}
+		}
+	})
+
+	for i := range gotOps {
+		o := gotOps[i]
+
+		t.Run("is", func(t *testing.T) {
+			if !o.Is(o) {
+				t.Errorf("Is returned false on self")
+				return
+			}
+			if !o.Is(wantOps[i]) {
+				t.Errorf("%s: inserted %#v, want %#v",
+					fn,
+					o, wantOps[i])
+				return
+			}
+		})
+
+		t.Run("criteria", func(t *testing.T) {
+			testCases := []struct {
+				name string
+				ec   *Criteria
+				want bool
+			}{
+				{"nil", nil, ptc.et != User},
+				{"self", newCriteria(ptc.et), true},
+				{"all", newCriteria(EWayland | EX11 | EDBus | EPulse | User | Process), true},
+				{"enablements", newCriteria(EWayland | EX11 | EDBus | EPulse), ptc.et != User && ptc.et != Process},
+			}
+
+			for _, tc := range testCases {
+				t.Run(tc.name, func(t *testing.T) {
+					if got := tc.ec.hasType(o); got != tc.want {
+						t.Errorf("hasType: got %v, want %v",
+							got, tc.want)
+					}
+				})
+			}
+		})
+	}
+}
+
+func newCriteria(e Enablement) *Criteria { return (*Criteria)(&e) }

system/tmpfiles.go

@@ -40,26 +40,20 @@ func (t *Tmpfile) apply(*I) error {
 	}
 
 	if b, err := os.Stat(t.src); err != nil {
-		return wrapErrSuffix(err,
+		return newOpError("tmpfile", err, false)
-			fmt.Sprintf("cannot stat %q:", t.src))
 	} else {
 		if b.IsDir() {
-			return wrapErrSuffix(syscall.EISDIR,
+			return newOpError("tmpfile", &os.PathError{Op: "stat", Path: t.src, Err: syscall.EISDIR}, false)
-				fmt.Sprintf("%q is a directory", t.src))
 		}
 		if s := b.Size(); s > t.n {
-			return wrapErrSuffix(syscall.ENOMEM,
+			return newOpError("tmpfile", &os.PathError{Op: "stat", Path: t.src, Err: syscall.ENOMEM}, false)
-				fmt.Sprintf("file %q is too long: %d > %d",
-					t.src, s, t.n))
 		}
 	}
 
 	if f, err := os.Open(t.src); err != nil {
-		return wrapErrSuffix(err,
+		return newOpError("tmpfile", err, false)
-			fmt.Sprintf("cannot open %q:", t.src))
 	} else if _, err = io.CopyN(t.buf, f, t.n); err != nil {
-		return wrapErrSuffix(err,
+		return newOpError("tmpfile", err, false)
-			fmt.Sprintf("cannot read from %q:", t.src))
 	}
 
 	*t.payload = t.buf.Bytes()

system/wayland.go

@@ -31,35 +31,31 @@ func (w *Wayland) Type() Enablement { return Process }
 
 func (w *Wayland) apply(sys *I) error {
 	if w.sync == nil {
-		// this is a misuse of the API; do not return an error message
+		// this is a misuse of the API; do not return a wrapped error
 		return errors.New("invalid sync")
 	}
 
 	// the Wayland op is not repeatable
 	if *w.sync != nil {
-		// this is a misuse of the API; do not return an error message
+		// this is a misuse of the API; do not return a wrapped error
 		return errors.New("attempted to attach multiple wayland sockets")
 	}
 
 	if err := w.conn.Attach(w.src); err != nil {
-		// make console output less nasty
+		return newOpError("wayland", err, false)
-		if errors.Is(err, os.ErrNotExist) {
-			err = os.ErrNotExist
-		}
-		return wrapErrSuffix(err,
-			fmt.Sprintf("cannot attach to wayland on %q:", w.src))
 	} else {
 		msg.Verbosef("wayland attached on %q", w.src)
 	}
 
 	if sp, err := w.conn.Bind(w.dst, w.appID, w.instanceID); err != nil {
-		return wrapErrSuffix(err,
+		return newOpError("wayland", err, false)
-			fmt.Sprintf("cannot bind to socket on %q:", w.dst))
 	} else {
 		*w.sync = sp
 		msg.Verbosef("wayland listening on %q", w.dst)
-		return wrapErrSuffix(errors.Join(os.Chmod(w.dst, 0), acl.Update(w.dst, sys.uid, acl.Read, acl.Write, acl.Execute)),
+		if err = os.Chmod(w.dst, 0); err != nil {
-			fmt.Sprintf("cannot chmod socket on %q:", w.dst))
+			return newOpError("wayland", err, false)
+		}
+		return newOpError("wayland", acl.Update(w.dst, sys.uid, acl.Read, acl.Write, acl.Execute), false)
 	}
 }
 
@@ -67,12 +63,11 @@ func (w *Wayland) revert(_ *I, ec *Criteria) error {
 	if ec.hasType(w) {
 		msg.Verbosef("removing wayland socket on %q", w.dst)
 		if err := os.Remove(w.dst); err != nil && !errors.Is(err, os.ErrNotExist) {
-			return err
+			return newOpError("wayland", err, true)
 		}
 
 		msg.Verbosef("detaching from wayland on %q", w.src)
-		return wrapErrSuffix(w.conn.Close(),
+		return newOpError("wayland", w.conn.Close(), true)
-			fmt.Sprintf("cannot detach from wayland on %q:", w.src))
 	} else {
 		msg.Verbosef("skipping wayland cleanup on %q", w.dst)
 		return nil

system/xhost.go

@@ -1,8 +1,6 @@
 package system
 
 import (
-	"fmt"
-
 	"hakurei.app/system/internal/xcb"
 )
 
@@ -18,36 +16,25 @@ func (sys *I) ChangeHosts(username string) *I {
 
 type XHost string
 
-func (x XHost) Type() Enablement {
+func (x XHost) Type() Enablement { return EX11 }
-	return EX11
-}
 
 func (x XHost) apply(*I) error {
 	msg.Verbosef("inserting entry %s to X11", x)
-	return wrapErrSuffix(xcb.ChangeHosts(xcb.HostModeInsert, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)),
+	return newOpError("xhost",
-		fmt.Sprintf("cannot insert entry %s to X11:", x))
+		xcb.ChangeHosts(xcb.HostModeInsert, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)), false)
 }
 
 func (x XHost) revert(_ *I, ec *Criteria) error {
 	if ec.hasType(x) {
 		msg.Verbosef("deleting entry %s from X11", x)
-		return wrapErrSuffix(xcb.ChangeHosts(xcb.HostModeDelete, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)),
+		return newOpError("xhost",
-			fmt.Sprintf("cannot delete entry %s from X11:", x))
+			xcb.ChangeHosts(xcb.HostModeDelete, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)), false)
 	} else {
 		msg.Verbosef("skipping entry %s in X11", x)
 		return nil
 	}
 }
 
-func (x XHost) Is(o Op) bool {
+func (x XHost) Is(o Op) bool   { x0, ok := o.(XHost); return ok && x == x0 }
-	x0, ok := o.(XHost)
+func (x XHost) Path() string   { return string(x) }
-	return ok && x == x0
+func (x XHost) String() string { return string("SI:localuser:" + x) }
-}
-
-func (x XHost) Path() string {
-	return string(x)
-}
-
-func (x XHost) String() string {
-	return string("SI:localuser:" + x)
-}