system/xhost: wrap revert error correctly

This otherwise creates a confusing error message on a revert failure.

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

5 files changed, 74 insertions, 101 deletions

diff --git a/system/dispatcher.go b/system/dispatcher.go
index 502c8e92..f8de2ff9 100644
--- a/system/dispatcher.go
+++ b/system/dispatcher.go
@@ -7,6 +7,7 @@ import (
 
 	"hakurei.app/system/acl"
 	"hakurei.app/system/dbus"
+	"hakurei.app/system/internal/xcb"
 )
 
 type osFile interface {
@@ -39,6 +40,9 @@ type syscallDispatcher interface {
 	// aclUpdate provides [acl.Update].
 	aclUpdate(name string, uid int, perms ...acl.Perm) error
 
+	// xcbChangeHosts provides [xcb.ChangeHosts].
+	xcbChangeHosts(mode xcb.HostMode, family xcb.Family, address string) error
+
 	// dbusAddress provides [dbus.Address].
 	dbusAddress() (session, system string)
 	// dbusFinalise provides [dbus.Finalise].
@@ -71,6 +75,10 @@ func (k direct) aclUpdate(name string, uid int, perms ...acl.Perm) error {
 	return acl.Update(name, uid, perms...)
 }
 
+func (k direct) xcbChangeHosts(mode xcb.HostMode, family xcb.Family, address string) error {
+	return xcb.ChangeHosts(mode, family, address)
+}
+
 func (k direct) dbusAddress() (session, system string) {
 	return dbus.Address()
 }
diff --git a/system/dispatcher_test.go b/system/dispatcher_test.go
index f798047c..7bd4e6db 100644
--- a/system/dispatcher_test.go
+++ b/system/dispatcher_test.go
@@ -13,6 +13,7 @@ import (
 	"hakurei.app/container/stub"
 	"hakurei.app/system/acl"
 	"hakurei.app/system/dbus"
+	"hakurei.app/system/internal/xcb"
 )
 
 // call initialises a [stub.Call].
@@ -280,6 +281,14 @@ func (k *kstub) aclUpdate(name string, uid int, perms ...acl.Perm) error {
 		stub.CheckArgReflect(k.Stub, "perms", perms, 2))
 }
 
+func (k *kstub) xcbChangeHosts(mode xcb.HostMode, family xcb.Family, address string) error {
+	k.Helper()
+	return k.Expects("xcbChangeHosts").Error(
+		stub.CheckArg(k.Stub, "mode", mode, 0),
+		stub.CheckArg(k.Stub, "family", family, 1),
+		stub.CheckArg(k.Stub, "address", address, 2))
+}
+
 func (k *kstub) dbusAddress() (session, system string) {
 	k.Helper()
 	ret := k.Expects("dbusAddress").Ret.([2]string)
diff --git a/system/output_test.go b/system/output_test.go
index b091f5ef..80abcf8d 100644
--- a/system/output_test.go
+++ b/system/output_test.go
@@ -158,70 +158,3 @@ func TestPrintJoinedError(t *testing.T) {
 		})
 	}
 }
-
-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   Enablement
-				want bool
-			}{
-				{"nil", 0xff, ptc.et != User},
-				{"self", ptc.et, true},
-				{"all", EWayland | EX11 | EDBus | EPulse | User | Process, true},
-				{"enablements", EWayland | EX11 | EDBus | EPulse, ptc.et != User && ptc.et != Process},
-			}
-
-			for _, tc := range testCases {
-				t.Run(tc.name, func(t *testing.T) {
-					var criteria *Criteria
-					if tc.ec != 0xff {
-						criteria = (*Criteria)(&tc.ec)
-					}
-					if got := criteria.hasType(o.Type()); got != tc.want {
-						t.Errorf("hasType: got %v, want %v",
-							got, tc.want)
-					}
-				})
-			}
-		})
-	}
-}
diff --git a/system/xhost.go b/system/xhost.go
index cdec1290..4ee49931 100644
--- a/system/xhost.go
+++ b/system/xhost.go
@@ -15,23 +15,23 @@ type xhostOp string
 
 func (x xhostOp) Type() Enablement { return EX11 }
 
-func (x xhostOp) apply(*I) error {
-	msg.Verbosef("inserting entry %s to X11", x)
+func (x xhostOp) apply(sys *I) error {
+	sys.verbosef("inserting entry %s to X11", x)
 	return newOpError("xhost",
-		xcb.ChangeHosts(xcb.HostModeInsert, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)), false)
+		sys.xcbChangeHosts(xcb.HostModeInsert, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)), false)
 }
 
-func (x xhostOp) revert(_ *I, ec *Criteria) error {
+func (x xhostOp) revert(sys *I, ec *Criteria) error {
 	if ec.hasType(x.Type()) {
-		msg.Verbosef("deleting entry %s from X11", x)
+		sys.verbosef("deleting entry %s from X11", x)
 		return newOpError("xhost",
-			xcb.ChangeHosts(xcb.HostModeDelete, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)), false)
+			sys.xcbChangeHosts(xcb.HostModeDelete, xcb.FamilyServerInterpreted, "localuser\x00"+string(x)), true)
 	} else {
-		msg.Verbosef("skipping entry %s in X11", x)
+		sys.verbosef("skipping entry %s in X11", x)
 		return nil
 	}
 }
 
 func (x xhostOp) Is(o Op) bool   { target, ok := o.(xhostOp); return ok && x == target }
-func (x xhostOp) Path() string   { return string(x) }
+func (x xhostOp) Path() string   { return "/tmp/.X11-unix" }
 func (x xhostOp) String() string { return string("SI:localuser:" + x) }
diff --git a/system/xhost_test.go b/system/xhost_test.go
index 32cb0636..68601e86 100644
--- a/system/xhost_test.go
+++ b/system/xhost_test.go
@@ -2,33 +2,56 @@ package system
 
 import (
 	"testing"
+
+	"hakurei.app/container/stub"
+	"hakurei.app/system/internal/xcb"
 )
 
-func TestChangeHosts(t *testing.T) {
-	testCases := []string{"chronos", "keyring", "cat", "kbd", "yonah"}
-	for _, tc := range testCases {
-		t.Run("append ChangeHosts operation for "+tc, func(t *testing.T) {
-			sys := New(t.Context(), 150)
-			sys.ChangeHosts(tc)
-			(&tcOp{EX11, tc}).test(t, sys.ops, []Op{
-				xhostOp(tc),
-			}, "ChangeHosts")
-		})
-	}
-}
+func TestXHostOp(t *testing.T) {
+	checkOpBehaviour(t, []opBehaviourTestCase{
+		{"xcbChangeHosts revert", 0xbeef, EX11, xhostOp("chronos"), []stub.Call{
+			call("verbosef", stub.ExpectArgs{"inserting entry %s to X11", []any{xhostOp("chronos")}}, nil, nil),
+			call("xcbChangeHosts", stub.ExpectArgs{xcb.HostMode(xcb.HostModeInsert), xcb.Family(xcb.FamilyServerInterpreted), "localuser\x00chronos"}, nil, stub.UniqueError(1)),
+		}, &OpError{Op: "xhost", Err: stub.UniqueError(1)}, nil, nil},
+
+		{"xcbChangeHosts revert", 0xbeef, EX11, xhostOp("chronos"), []stub.Call{
+			call("verbosef", stub.ExpectArgs{"inserting entry %s to X11", []any{xhostOp("chronos")}}, nil, nil),
+			call("xcbChangeHosts", stub.ExpectArgs{xcb.HostMode(xcb.HostModeInsert), xcb.Family(xcb.FamilyServerInterpreted), "localuser\x00chronos"}, nil, nil),
+		}, nil, []stub.Call{
+			call("verbosef", stub.ExpectArgs{"deleting entry %s from X11", []any{xhostOp("chronos")}}, nil, nil),
+			call("xcbChangeHosts", stub.ExpectArgs{xcb.HostMode(xcb.HostModeDelete), xcb.Family(xcb.FamilyServerInterpreted), "localuser\x00chronos"}, nil, stub.UniqueError(0)),
+		}, &OpError{Op: "xhost", Err: stub.UniqueError(0), Revert: true}},
+
+		{"success skip", 0xbeef, 0, xhostOp("chronos"), []stub.Call{
+			call("verbosef", stub.ExpectArgs{"inserting entry %s to X11", []any{xhostOp("chronos")}}, nil, nil),
+			call("xcbChangeHosts", stub.ExpectArgs{xcb.HostMode(xcb.HostModeInsert), xcb.Family(xcb.FamilyServerInterpreted), "localuser\x00chronos"}, nil, nil),
+		}, nil, []stub.Call{
+			call("verbosef", stub.ExpectArgs{"skipping entry %s in X11", []any{xhostOp("chronos")}}, nil, nil),
+		}, nil},
+
+		{"success", 0xbeef, EX11, xhostOp("chronos"), []stub.Call{
+			call("verbosef", stub.ExpectArgs{"inserting entry %s to X11", []any{xhostOp("chronos")}}, nil, nil),
+			call("xcbChangeHosts", stub.ExpectArgs{xcb.HostMode(xcb.HostModeInsert), xcb.Family(xcb.FamilyServerInterpreted), "localuser\x00chronos"}, nil, nil),
+		}, nil, []stub.Call{
+			call("verbosef", stub.ExpectArgs{"deleting entry %s from X11", []any{xhostOp("chronos")}}, nil, nil),
+			call("xcbChangeHosts", stub.ExpectArgs{xcb.HostMode(xcb.HostModeDelete), xcb.Family(xcb.FamilyServerInterpreted), "localuser\x00chronos"}, nil, nil),
+		}, nil},
+	})
+
+	checkOpsBuilder(t, "ChangeHosts", []opsBuilderTestCase{
+		{"xhost", 0xcafebabe, func(_ *testing.T, sys *I) {
+			sys.ChangeHosts("chronos")
+		}, []Op{
+			xhostOp("chronos"),
+		}, stub.Expect{}},
+	})
+
+	checkOpIs(t, []opIsTestCase{
+		{"differs", xhostOp("kbd"), xhostOp("chronos"), false},
+		{"equals", xhostOp("chronos"), xhostOp("chronos"), true},
+	})
 
-func TestXHost_String(t *testing.T) {
-	testCases := []struct {
-		username string
-		want     string
-	}{
-		{"chronos", "SI:localuser:chronos"},
-	}
-	for _, tc := range testCases {
-		t.Run(tc.want, func(t *testing.T) {
-			if got := xhostOp(tc.username).String(); got != tc.want {
-				t.Errorf("String() = %v, want %v", got, tc.want)
-			}
-		})
-	}
+	checkOpMeta(t, []opMetaTestCase{
+		{"xhost", xhostOp("chronos"), EX11, "/tmp/.X11-unix", "SI:localuser:chronos"},
+	})
 }