diff --git a/internal/agh/agh.go b/internal/agh/agh.go index adffa75f..e3c42b94 100644 --- a/internal/agh/agh.go +++ b/internal/agh/agh.go @@ -11,6 +11,10 @@ import ( "github.com/AdguardTeam/golibs/testutil/fakeos/fakeexec" ) +// DefaultOutputLimit is the default limit of bytes for commands' standard +// output and standard error. +const DefaultOutputLimit = 512 + // ConfigModifier defines an interface for updating the global configuration. type ConfigModifier interface { // Apply applies changes to the global configuration. @@ -27,11 +31,8 @@ var _ ConfigModifier = EmptyConfigModifier{} // Apply implements the [ConfigModifier] for EmptyConfigModifier. func (em EmptyConfigModifier) Apply(ctx context.Context) {} -// TODO(s.chzhen): !! Is there another way? -// -// TODO(s.chzhen): !! Docs, naming. -// -// TODO(s.chzhen): Move to aghtest once the import cycle is resolved. +// exitErr implements [executil.ExitCodeError] for tests to simulate non-zero +// process exit codes. type exitErr struct { code osutil.ExitCode } @@ -39,25 +40,37 @@ type exitErr struct { // type check var _ executil.ExitCodeError = exitErr{} +// Error implements the [executil.ExitCodeError] for exitErr. func (e exitErr) Error() (s string) { return fmt.Sprintf("exit code %d", e.code) } +// ExitCode implements the [executil.ExitCodeError] for exitErr. func (e exitErr) ExitCode() (code osutil.ExitCode) { return e.code } +// ExternalCommand is a fake command used by [NewMultipleCommandConstructor]. type ExternalCommand struct { - Err error - Cmd string - Out string + // Err is the error returned, if non-nil. + Err error + + // Cmd contains the command path and arguments. + Cmd string + + // Out is written to stdout if non-empty. + Out string + + // Code is returned as the exit code if non-zero. Code int } +// keyCommand builds a key for a command lookup. func keyCommand(path string, args []string) (k string) { return path + " " + strings.Join(args, " ") } +// parseCommand splits a command string into the executable path and args. func parseCommand(s string) (path string, args []string) { f := strings.Fields(s) if len(f) == 0 { @@ -68,7 +81,11 @@ func parseCommand(s string) (path string, args []string) { } // NewMultipleCommandConstructor is a helper function that returns a mock -// [executil.CommandConstructor] for tests. +// [executil.CommandConstructor] for tests that supports multiple commands. +// +// TODO(s.chzhen): Use this. +// +// TODO(s.chzhen): Move to aghtest once the import cycle is resolved. func NewMultipleCommandConstructor(cmds ...ExternalCommand) (cs executil.CommandConstructor) { table := make(map[string]ExternalCommand, len(cmds)) for _, ec := range cmds { diff --git a/internal/aghos/os.go b/internal/aghos/os.go index 1a7538ee..086894fb 100644 --- a/internal/aghos/os.go +++ b/internal/aghos/os.go @@ -93,6 +93,11 @@ func RunCommand( } // psArgs holds the default ps arguments to avoid per-call slice allocations. +// +// Don't use -C flag here since it's a feature of linux's ps +// implementation. Use POSIX-compatible flags instead. +// +// See https://github.com/AdguardTeam/AdGuardHome/issues/3457. var psArgs = []string{"-A", "-o", "pid=", "-o", "comm="} // PIDByCommand searches for process named command and returns its PID ignoring @@ -108,10 +113,6 @@ func PIDByCommand( l.DebugContext(ctx, "executing", "cmd", psCmd, "args", psArgs) - // Don't use -C flag here since it's a feature of linux's ps - // implementation. Use POSIX-compatible flags instead. - // - // See https://github.com/AdguardTeam/AdGuardHome/issues/3457. stdoutBuf := bytes.Buffer{} err = executil.Run( ctx, diff --git a/internal/arpdb/arpdb.go b/internal/arpdb/arpdb.go index 7124c8ab..7f6c1a1c 100644 --- a/internal/arpdb/arpdb.go +++ b/internal/arpdb/arpdb.go @@ -17,6 +17,7 @@ import ( "github.com/AdguardTeam/golibs/netutil" "github.com/AdguardTeam/golibs/osutil" "github.com/AdguardTeam/golibs/osutil/executil" + "github.com/AdguardTeam/golibs/service" ) // Variables and functions to substitute in tests. @@ -28,8 +29,8 @@ var ( // Interface stores and refreshes the network neighborhood reported by ARP // (Address Resolution Protocol). type Interface interface { - // Refresh updates the stored data. It must be safe for concurrent use. - Refresh() (err error) + // Refresher updates the stored data. It must be safe for concurrent use. + service.Refresher // Neighbors returnes the last set of data reported by ARP. Both the method // and it's result must be safe for concurrent use. @@ -49,7 +50,7 @@ var _ Interface = Empty{} // Refresh implements the [Interface] interface for EmptyARPContainer. It does // nothing and always returns nil error. -func (Empty) Refresh() (err error) { return nil } +func (Empty) Refresh(_ context.Context) (err error) { return nil } // Neighbors implements the [Interface] interface for EmptyARPContainer. It // always returns nil. @@ -174,13 +175,12 @@ type cmdARPDB struct { var _ Interface = (*cmdARPDB)(nil) // Refresh implements the [Interface] interface for *cmdARPDB. -func (arp *cmdARPDB) Refresh() (err error) { +func (arp *cmdARPDB) Refresh(ctx context.Context) (err error) { defer func() { err = errors.Annotate(err, "cmd arpdb: %w") }() var stdout bytes.Buffer err = executil.Run( - // TODO(s.chzhen): Pass context. - context.TODO(), + ctx, arp.cmdCons, &executil.CommandConfig{ Path: arp.cmd, @@ -237,11 +237,11 @@ func newARPDBs(arps ...Interface) (arp *arpdbs) { var _ Interface = (*arpdbs)(nil) // Refresh implements the [Interface] interface for *arpdbs. -func (arp *arpdbs) Refresh() (err error) { +func (arp *arpdbs) Refresh(ctx context.Context) (err error) { var errs []error for _, a := range arp.arps { - err = a.Refresh() + err = a.Refresh(ctx) if err != nil { errs = append(errs, err) diff --git a/internal/arpdb/arpdb_internal_test.go b/internal/arpdb/arpdb_internal_test.go index 092bd475..e9f307ae 100644 --- a/internal/arpdb/arpdb_internal_test.go +++ b/internal/arpdb/arpdb_internal_test.go @@ -1,12 +1,14 @@ package arpdb import ( + "context" "io/fs" "net" "net/netip" "os" "sync" "testing" + "time" "github.com/AdguardTeam/AdGuardHome/internal/agh" "github.com/AdguardTeam/golibs/errors" @@ -16,6 +18,9 @@ import ( "github.com/stretchr/testify/require" ) +// testTimeout is a common timeout for tests. +const testTimeout = 1 * time.Second + // testdata is the filesystem containing data for testing the package. var testdata fs.FS = os.DirFS("./testdata") @@ -33,7 +38,7 @@ func Test_New(t *testing.T) { // TestARPDB is the mock implementation of [Interface] to use in tests. type TestARPDB struct { - OnRefresh func() (err error) + OnRefresh func(ctx context.Context) (err error) OnNeighbors func() (ns []Neighbor) } @@ -41,8 +46,8 @@ type TestARPDB struct { var _ Interface = (*TestARPDB)(nil) // Refresh implements the [Interface] interface for *TestARPDB. -func (arp *TestARPDB) Refresh() (err error) { - return arp.OnRefresh() +func (arp *TestARPDB) Refresh(ctx context.Context) (err error) { + return arp.OnRefresh(ctx) } // Neighbors implements the [Interface] interface for *TestARPDB. @@ -60,13 +65,17 @@ func Test_NewARPDBs(t *testing.T) { } succDB := &TestARPDB{ - OnRefresh: func() (err error) { succRefrCount++; return nil }, + OnRefresh: func(_ context.Context) (err error) { succRefrCount++; return nil }, OnNeighbors: func() (ns []Neighbor) { return []Neighbor{{Name: "abc", IP: knownIP, MAC: knownMAC}} }, } failDB := &TestARPDB{ - OnRefresh: func() (err error) { failRefrCount++; return errors.Error("refresh failed") }, + OnRefresh: func(_ context.Context) (err error) { + failRefrCount++ + + return errors.Error("refresh failed") + }, OnNeighbors: func() (ns []Neighbor) { return nil }, } @@ -74,7 +83,7 @@ func Test_NewARPDBs(t *testing.T) { t.Cleanup(clnp) a := newARPDBs(succDB, failDB) - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Equal(t, 1, succRefrCount) @@ -86,7 +95,7 @@ func Test_NewARPDBs(t *testing.T) { t.Cleanup(clnp) a := newARPDBs(failDB, succDB) - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Equal(t, 1, succRefrCount) @@ -100,7 +109,7 @@ func Test_NewARPDBs(t *testing.T) { wantMsg := "each arpdb failed: refresh failed\nrefresh failed" a := newARPDBs(failDB, failDB) - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.Error(t, err) testutil.AssertErrorMsg(t, wantMsg, err) @@ -114,7 +123,7 @@ func Test_NewARPDBs(t *testing.T) { shouldFail := false unstableDB := &TestARPDB{ - OnRefresh: func() (err error) { + OnRefresh: func(_ context.Context) (err error) { if shouldFail { err = errors.Error("unstable failed") } @@ -133,21 +142,21 @@ func Test_NewARPDBs(t *testing.T) { a := newARPDBs(unstableDB, succDB) // Unstable ARPDB should refresh successfully. - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Zero(t, succRefrCount) assert.NotEmpty(t, a.Neighbors()) // Unstable ARPDB should fail and the succDB should be used. - err = a.Refresh() + err = a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Equal(t, 1, succRefrCount) assert.NotEmpty(t, a.Neighbors()) // Unstable ARPDB should refresh successfully again. - err = a.Refresh() + err = a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Equal(t, 1, succRefrCount) @@ -156,7 +165,7 @@ func Test_NewARPDBs(t *testing.T) { t.Run("empty", func(t *testing.T) { a := newARPDBs() - require.NoError(t, a.Refresh()) + require.NoError(t, a.Refresh(testutil.ContextWithTimeout(t, testTimeout))) assert.Empty(t, a.Neighbors()) }) @@ -176,7 +185,7 @@ func TestCmdARPDB_arpa(t *testing.T) { t.Run("arp_a", func(t *testing.T) { a.cmdCons = agh.NewCommandConstructor("cmd", 0, arpAOutput, nil) - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Equal(t, wantNeighs, a.Neighbors()) @@ -185,14 +194,14 @@ func TestCmdARPDB_arpa(t *testing.T) { t.Run("runcmd_error", func(t *testing.T) { a.cmdCons = agh.NewCommandConstructor("cmd", 0, "", errors.Error("can't run")) - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) testutil.AssertErrorMsg(t, "cmd arpdb: running command: running: can't run", err) }) t.Run("bad_code", func(t *testing.T) { a.cmdCons = agh.NewCommandConstructor("cmd", 1, "", nil) - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) testutil.AssertErrorMsg( t, "cmd arpdb: running command: unexpected exit code 1", @@ -203,7 +212,7 @@ func TestCmdARPDB_arpa(t *testing.T) { t.Run("empty", func(t *testing.T) { a.cmdCons = agh.NewCommandConstructor("cmd", 0, "", nil) - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Empty(t, a.Neighbors()) @@ -216,7 +225,7 @@ func TestEmptyARPDB(t *testing.T) { t.Run("refresh", func(t *testing.T) { var err error require.NotPanics(t, func() { - err = a.Refresh() + err = a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) }) assert.NoError(t, err) diff --git a/internal/arpdb/arpdb_linux.go b/internal/arpdb/arpdb_linux.go index 518e7ea4..a455411d 100644 --- a/internal/arpdb/arpdb_linux.go +++ b/internal/arpdb/arpdb_linux.go @@ -4,6 +4,7 @@ package arpdb import ( "bufio" + "context" "fmt" "io/fs" "log/slog" @@ -76,7 +77,7 @@ type fsysARPDB struct { var _ Interface = (*fsysARPDB)(nil) // Refresh implements the [Interface] interface for *fsysARPDB. -func (arp *fsysARPDB) Refresh() (err error) { +func (arp *fsysARPDB) Refresh(_ context.Context) (err error) { var f fs.File f, err = arp.fsys.Open(arp.filename) if err != nil { diff --git a/internal/arpdb/arpdb_linux_internal_test.go b/internal/arpdb/arpdb_linux_internal_test.go index b2e2f022..4d9c0083 100644 --- a/internal/arpdb/arpdb_linux_internal_test.go +++ b/internal/arpdb/arpdb_linux_internal_test.go @@ -11,6 +11,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/agh" "github.com/AdguardTeam/golibs/logutil/slogutil" + "github.com/AdguardTeam/golibs/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -55,7 +56,7 @@ func TestFSysARPDB(t *testing.T) { filename: "proc_net_arp", } - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) ns := a.Neighbors() @@ -76,7 +77,7 @@ func TestCmdARPDB_linux(t *testing.T) { }, } - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Equal(t, wantNeighs, a.Neighbors()) @@ -94,7 +95,7 @@ func TestCmdARPDB_linux(t *testing.T) { ns: make([]Neighbor, 0), }, } - err := a.Refresh() + err := a.Refresh(testutil.ContextWithTimeout(t, testTimeout)) require.NoError(t, err) assert.Equal(t, wantNeighs, a.Neighbors()) diff --git a/internal/client/storage.go b/internal/client/storage.go index 4ec327ad..42953d90 100644 --- a/internal/client/storage.go +++ b/internal/client/storage.go @@ -249,7 +249,7 @@ func (s *Storage) addFromSystemARP(ctx context.Context) { s.mu.Lock() defer s.mu.Unlock() - if err := s.arpDB.Refresh(); err != nil { + if err := s.arpDB.Refresh(ctx); err != nil { s.arpDB = arpdb.Empty{} s.logger.ErrorContext(ctx, "refreshing arp container", slogutil.KeyError, err) diff --git a/internal/client/storage_test.go b/internal/client/storage_test.go index 658163b8..32f34c00 100644 --- a/internal/client/storage_test.go +++ b/internal/client/storage_test.go @@ -1,6 +1,7 @@ package client_test import ( + "context" "net" "net/netip" "runtime" @@ -18,6 +19,7 @@ import ( "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/hostsfile" "github.com/AdguardTeam/golibs/logutil/slogutil" + "github.com/AdguardTeam/golibs/service" "github.com/AdguardTeam/golibs/testutil" "github.com/AdguardTeam/golibs/testutil/faketime" "github.com/AdguardTeam/golibs/timeutil" @@ -63,8 +65,8 @@ func (c *testHostsContainer) Upd() (updates <-chan *hostsfile.DefaultStorage) { // Interface stores and refreshes the network neighborhood reported by ARP // (Address Resolution Protocol). type Interface interface { - // Refresh updates the stored data. It must be safe for concurrent use. - Refresh() (err error) + // Refresher updates the stored data. It must be safe for concurrent use. + service.Refresher // Neighbors returnes the last set of data reported by ARP. Both the method // and it's result must be safe for concurrent use. @@ -73,7 +75,7 @@ type Interface interface { // testARPDB is a mock implementation of the [arpdb.Interface]. type testARPDB struct { - onRefresh func() (err error) + onRefresh func(ctx context.Context) (err error) onNeighbors func() (ns []arpdb.Neighbor) } @@ -81,8 +83,8 @@ type testARPDB struct { var _ arpdb.Interface = (*testARPDB)(nil) // Refresh implements the [arpdb.Interface] interface for *testARP. -func (c *testARPDB) Refresh() (err error) { - return c.onRefresh() +func (c *testARPDB) Refresh(ctx context.Context) (err error) { + return c.onRefresh(ctx) } // Neighbors implements the [arpdb.Interface] interface for *testARP. @@ -218,7 +220,7 @@ func TestStorage_Add_arp(t *testing.T) { ) a := &testARPDB{ - onRefresh: func() (err error) { return nil }, + onRefresh: func(_ context.Context) (err error) { return nil }, onNeighbors: func() (ns []arpdb.Neighbor) { mu.Lock() defer mu.Unlock() @@ -392,7 +394,7 @@ func TestClientsDHCP(t *testing.T) { arpCh := make(chan []arpdb.Neighbor, 1) arpDB := &testARPDB{ - onRefresh: func() (err error) { return nil }, + onRefresh: func(_ context.Context) (err error) { return nil }, onNeighbors: func() (ns []arpdb.Neighbor) { select { case ns = <-arpCh: diff --git a/internal/home/controlinstall.go b/internal/home/controlinstall.go index c9d6e912..b5b640cb 100644 --- a/internal/home/controlinstall.go +++ b/internal/home/controlinstall.go @@ -1,7 +1,6 @@ package home import ( - "bytes" "context" "encoding/json" "fmt" @@ -256,13 +255,12 @@ func checkDNSStubListener(ctx context.Context, l *slog.Logger) (ok bool) { for _, cmd := range cmds { l.DebugContext(ctx, "executing", "cmd", cmd.Key, "args", cmd.Value) - err := executil.Run( + err := executil.RunWithPeek( ctx, executil.SystemCommandConstructor{}, - &executil.CommandConfig{ - Path: cmd.Key, - Args: cmd.Value, - }, + agh.DefaultOutputLimit, + cmd.Key, + cmd.Value..., ) if err != nil { l.InfoContext(ctx, "execution failed", "cmd", cmd.Key, slogutil.KeyError, err) @@ -308,23 +306,16 @@ func disableDNSStubListener(ctx context.Context, l *slog.Logger) (err error) { systemctlCmd = "systemctl" ) - var ( - systemctlArgs = []string{"reload-or-restart", "systemd-resolved"} - systemctlStdout bytes.Buffer - systemctlStderr bytes.Buffer - ) + systemctlArgs := []string{"reload-or-restart", "systemd-resolved"} l.DebugContext(ctx, "executing", "cmd", systemctlCmd, "args", systemctlArgs) - err = executil.Run( + err = executil.RunWithPeek( ctx, executil.SystemCommandConstructor{}, - &executil.CommandConfig{ - Path: systemctlCmd, - Args: systemctlArgs, - Stdout: &systemctlStdout, - Stderr: &systemctlStderr, - }, + agh.DefaultOutputLimit, + systemctlCmd, + systemctlArgs..., ) if err != nil { return fmt.Errorf("executing cmd: %w", err) diff --git a/internal/home/service_linux.go b/internal/home/service_linux.go index 6b43630c..d3534a42 100644 --- a/internal/home/service_linux.go +++ b/internal/home/service_linux.go @@ -156,9 +156,7 @@ var _ service.Service = (*systemdService)(nil) // Status implements the [service.Service] interface for *systemdService. func (s *systemdService) Status() (status service.Status, err error) { - const ( - systemctlCmd = "systemctl" - ) + const systemctlCmd = "systemctl" var ( systemctlArgs = []string{"show", s.unitName}