From f5abce9df5727904a1daa246025a87c4bfe62553 Mon Sep 17 00:00:00 2001 From: Ophestra Date: Sat, 30 Aug 2025 22:49:12 +0900 Subject: [PATCH] system: wrap op errors This passes more information allowing for better error handling. This eliminates generic WrapErr from system. Signed-off-by: Ophestra --- system/acl.go | 6 +- system/dbus.go | 31 +++--- system/link.go | 6 +- system/mkdir.go | 19 ++-- system/op.go | 2 +- system/op_internal_test.go | 68 ------------- system/output.go | 58 +++++++++++- system/output_test.go | 189 +++++++++++++++++++++++++++++++++++++ system/tmpfiles.go | 16 +--- system/wayland.go | 25 ++--- system/xhost.go | 29 ++---- 11 files changed, 297 insertions(+), 152 deletions(-) delete mode 100644 system/op_internal_test.go create mode 100644 system/output_test.go diff --git a/system/acl.go b/system/acl.go index 5d48c6c..82777b9 100644 --- a/system/acl.go +++ b/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...), - fmt.Sprintf("cannot apply ACL entry to %q:", a.path)) + return newOpError("acl", acl.Update(a.path, sys.uid, a.perms...), false) } 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, - fmt.Sprintf("cannot strip ACL entry from %q:", a.path)) + return newOpError("acl", err, true) } else { msg.Verbose("skipping ACL", a) return nil diff --git a/system/dbus.go b/system/dbus.go index f6f4a2a..b891089 100644 --- a/system/dbus.go +++ b/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, - "attempted to create message bus proxy args without session bus config") + return nil, newOpErrorMessage("dbus", ErrDBusConfig, + "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, - "cannot start message bus proxy:") + return newOpErrorMessage("dbus", err, + 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 { - return "(dbus proxy)" -} - -func (d *DBus) String() string { - return d.proxy.String() -} +func (d *DBus) Path() string { return "(dbus proxy)" } +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 { - log.Println("(dbus) " + msg) + for _, m := range s.msgbuf { + log.Println("(dbus) " + m) } s.mu.RUnlock() } diff --git a/system/link.go b/system/link.go index f307e67..022ee4e 100644 --- a/system/link.go +++ b/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), - fmt.Sprintf("cannot link %q:", l.dst)) + return newOpError("hardlink", os.Link(l.src, l.dst), false) } 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), - fmt.Sprintf("cannot remove hard link %q:", l.dst)) + return newOpError("hardlink", os.Remove(l.dst), true) } else { msg.Verbosef("skipping hard link %q", l.dst) return nil diff --git a/system/mkdir.go b/system/mkdir.go index 6e7a55f..51fc274 100644 --- a/system/mkdir.go +++ b/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 !errors.Is(err, os.ErrExist) { - return wrapErrSuffix(err, - fmt.Sprintf("cannot create directory %q:", m.path)) + if err := os.Mkdir(m.path, m.perm); err != nil { + if !errors.Is(err, os.ErrExist) { + return newOpError("mkdir", err, false) + } + // 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), - fmt.Sprintf("cannot remove ephemeral directory %q:", m.path)) + return newOpError("mkdir", os.Remove(m.path), true) } else { msg.Verbose("skipping ephemeral directory", m) return nil diff --git a/system/op.go b/system/op.go index 10e95c1..25f3fa2 100644 --- a/system/op.go +++ b/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) } } }() diff --git a/system/op_internal_test.go b/system/op_internal_test.go deleted file mode 100644 index 4d43309..0000000 --- a/system/op_internal_test.go +++ /dev/null @@ -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) } diff --git a/system/output.go b/system/output.go index 303f749..ec1802b 100644 --- a/system/output.go +++ b/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()) + } + } } diff --git a/system/output_test.go b/system/output_test.go new file mode 100644 index 0000000..5ad219b --- /dev/null +++ b/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) } diff --git a/system/tmpfiles.go b/system/tmpfiles.go index 6ed6e05..44d170f 100644 --- a/system/tmpfiles.go +++ b/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, - fmt.Sprintf("cannot stat %q:", t.src)) + return newOpError("tmpfile", err, false) } else { if b.IsDir() { - return wrapErrSuffix(syscall.EISDIR, - fmt.Sprintf("%q is a directory", t.src)) + return newOpError("tmpfile", &os.PathError{Op: "stat", Path: t.src, Err: syscall.EISDIR}, false) } if s := b.Size(); s > t.n { - return wrapErrSuffix(syscall.ENOMEM, - fmt.Sprintf("file %q is too long: %d > %d", - t.src, s, t.n)) + return newOpError("tmpfile", &os.PathError{Op: "stat", Path: t.src, Err: syscall.ENOMEM}, false) } } if f, err := os.Open(t.src); err != nil { - return wrapErrSuffix(err, - fmt.Sprintf("cannot open %q:", t.src)) + return newOpError("tmpfile", err, false) } else if _, err = io.CopyN(t.buf, f, t.n); err != nil { - return wrapErrSuffix(err, - fmt.Sprintf("cannot read from %q:", t.src)) + return newOpError("tmpfile", err, false) } *t.payload = t.buf.Bytes() diff --git a/system/wayland.go b/system/wayland.go index 4705acb..f08f58b 100644 --- a/system/wayland.go +++ b/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 - if errors.Is(err, os.ErrNotExist) { - err = os.ErrNotExist - } - return wrapErrSuffix(err, - fmt.Sprintf("cannot attach to wayland on %q:", w.src)) + return newOpError("wayland", err, false) } 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, - fmt.Sprintf("cannot bind to socket on %q:", w.dst)) + return newOpError("wayland", err, false) } 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)), - fmt.Sprintf("cannot chmod socket on %q:", w.dst)) + if err = os.Chmod(w.dst, 0); err != nil { + 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(), - fmt.Sprintf("cannot detach from wayland on %q:", w.src)) + return newOpError("wayland", w.conn.Close(), true) } else { msg.Verbosef("skipping wayland cleanup on %q", w.dst) return nil diff --git a/system/xhost.go b/system/xhost.go index 7bbc372..27b2fc6 100644 --- a/system/xhost.go +++ b/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 { - return EX11 -} +func (x XHost) Type() Enablement { 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)), - fmt.Sprintf("cannot insert entry %s to X11:", x)) + return newOpError("xhost", + 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)), - fmt.Sprintf("cannot delete entry %s from X11:", x)) + return newOpError("xhost", + 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 { - x0, ok := o.(XHost) - return ok && x == x0 -} - -func (x XHost) Path() string { - return string(x) -} - -func (x XHost) String() string { - return string("SI:localuser:" + x) -} +func (x XHost) Is(o Op) bool { x0, ok := o.(XHost); return ok && x == x0 } +func (x XHost) Path() string { return string(x) } +func (x XHost) String() string { return string("SI:localuser:" + x) }