all: imp code, docs

CHANGELOG.md

@@ -27,7 +27,7 @@ and this project adheres to
 
 ### Changed
 
-- ARP-based client's information source now uses fallback implementations
+- Improved detection of runtime clients through more resilient ARP processing
   ([#3597]).
 - Domain-specific private reverse DNS upstream servers are now validated to
   allow only `*.in-addr.arpa` and `*.ip6.arpa` domains pointing to

internal/aghnet/arpdb.go

@@ -13,12 +13,12 @@ import (
 	"github.com/AdguardTeam/golibs/netutil"
 )
 
-// ARPDB
+// ARPDB: The Network Neighborhood Database
 
-// ARPDB stores and refreshes the network neighborhood reported by ARP.
+// ARPDB stores and refreshes the network neighborhood reported by ARP (Address
+// Resolution Protocol).
 type ARPDB interface {
-	// Refresh tries to update the stored data.  It must be safe for concurrent
-	// use.
+	// Refresh updates the stored data.  It must be safe for concurrent use.
 	Refresh() (err error)
 
 	// Neighbors returnes the last set of data reported by ARP.  Both the method
@@ -46,10 +46,12 @@ type EmptyARPDB struct{}
 // type check
 var _ ARPDB = EmptyARPDB{}
 
-// Refresh implements the ARPDB interface for EmptyARPContainer.
+// Refresh implements the ARPDB interface for EmptyARPContainer.  It does
+// nothing and always returns nil error.
 func (EmptyARPDB) Refresh() (err error) { return nil }
 
-// Neighbors implements the ARPDB interface for EmptyARPContainer.
+// Neighbors implements the ARPDB interface for EmptyARPContainer.  It always
+// returns nil.
 func (EmptyARPDB) Neighbors() (ns []Neighbor) { return nil }
 
 // ARPDB Helper Types
@@ -116,27 +118,27 @@ func (ns *neighs) reset(with []Neighbor) {
 
 // Command ARPDB
 
-// parseF parses the text from sc as if it'd be an output of some ARP-related
+// parseFunc parses the text from sc as if it'd be an output of some ARP-related
 // command.  lenHint is a hint for the size of the allocated slice of Neighbors.
-type parseF func(sc *bufio.Scanner, lenHint int) (ns []Neighbor)
+type parseFunc func(sc *bufio.Scanner, lenHint int) (ns []Neighbor)
 
-type runcmdF func() (r io.Reader, err error)
+type runcmdFunc func() (r io.Reader, err error)
 
 // cmdARPDB is the implementation of the ARPDB that uses command line to
 // retrieve data.
 type cmdARPDB struct {
-	parse  parseF
-	runcmd runcmdF
+	parse  parseFunc
+	runcmd runcmdFunc
 	ns     *neighs
 }
 
 // type check
 var _ ARPDB = (*cmdARPDB)(nil)
 
-// rc runs the cmd with it's args and returns the result wrapped with io.Reader.
-// The error is returned either if the exit code retured by command not equals 0
-// or the execution itself failed.
-func rc(cmd string, args ...string) (r io.Reader, err error) {
+// runCmd runs the cmd with it's args and returns the result wrapped to be an
+// io.Reader.  The error is returned either if the exit code retured by command
+// not equals 0 or the execution itself failed.
+func runCmd(cmd string, args ...string) (r io.Reader, err error) {
 	var code int
 	var out string
 	code, out, err = aghos.RunCommand(cmd, args...)
@@ -177,31 +179,37 @@ func (arp *cmdARPDB) Neighbors() (ns []Neighbor) {
 
 // Composite ARPDB
 
-// compARPDB is the ARPDB that combines several ARPDB implementations and
+// arpdbs is the ARPDB that combines several ARPDB implementations and
 // consequently switches between those.
-type compARPDB struct {
+type arpdbs struct {
+	// arps is the set of ARPDB implementations to range through.
 	arps []ARPDB
+	// last is the last succeeded ARPDB index.
 	last int
 }
 
-func newCompARPDB(arps ...ARPDB) (arp *compARPDB) {
-	return &compARPDB{
+func newARPDBs(arps ...ARPDB) (arp *arpdbs) {
+	return &arpdbs{
 		arps: arps,
 		last: 0,
 	}
 }
 
 // type check
-var _ ARPDB = (*compARPDB)(nil)
+var _ ARPDB = (*arpdbs)(nil)
 
-// Refresh implements the ARPDB interface for *compARPDB.
-func (arp *compARPDB) Refresh() (err error) {
-	l := len(arp.arps)
+// Refresh implements the ARPDB interface for *arpdbs.
+func (arp *arpdbs) Refresh() (err error) {
 	var errs []error
-	for i, last := 0, arp.last; i < l; i, last = i+1, (arp.last+1)%l {
-		err = arp.arps[last].Refresh()
+	l := len(arp.arps)
+	// Start from the last succeeded implementation.
+	for i := 0; i < l; i++ {
+		cur := (arp.last + i) % l
+		err = arp.arps[cur].Refresh()
 		if err == nil {
-			arp.last = last
+			// The succeeded implementation found so update the last succeeded
+			// index.
+			arp.last = cur
 
 			return nil
 		}
@@ -210,14 +218,14 @@ func (arp *compARPDB) Refresh() (err error) {
 	}
 
 	if len(errs) > 0 {
-		err = errors.List("all implementations failed", errs...)
+		err = errors.List("each arpdb failed", errs...)
 	}
 
 	return err
 }
 
-// Neighbors implements the ARPDB interface for *compARPDB.
-func (arp *compARPDB) Neighbors() (ns []Neighbor) {
+// Neighbors implements the ARPDB interface for *arpdbs.
+func (arp *arpdbs) Neighbors() (ns []Neighbor) {
 	if l := len(arp.arps); l > 0 && arp.last < l {
 		return arp.arps[arp.last].Neighbors()
 	}

internal/aghnet/arpdb_bsd.go

@@ -57,8 +57,8 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 		}
 
 		host := fields[0]
-		if verr := netutil.ValidateDomainName(host); verr != nil {
-			log.Debug("parsing arp output: %s", verr)
+		if err := netutil.ValidateDomainName(host); err != nil {
+			log.Debug("parsing arp output: %s", err)
 		} else {
 			n.Name = host
 		}

internal/aghnet/arpdb_linux.go

@@ -18,25 +18,25 @@ import (
 	"github.com/AdguardTeam/golibs/stringutil"
 )
 
-func newARPDB() (arp *compARPDB) {
+func newARPDB() (arp *arpdbs) {
 	// Use the common storage among the implementations.
 	ns := &neighs{
 		mu: &sync.RWMutex{},
 		ns: make([]Neighbor, 0),
 	}
 
-	var parseArpAFunc parseF
+	var parseF parseFunc
 	if aghos.IsOpenWrt() {
-		parseArpAFunc = parseArpAWrt
+		parseF = parseArpAWrt
 	} else {
-		parseArpAFunc = parseArpA
+		parseF = parseArpA
 	}
 
-	return newCompARPDB(
+	return newARPDBs(
 		// Try /proc/net/arp first.
 		&fsysARPDB{ns: ns, fsys: aghos.RootDirFS(), filename: "proc/net/arp"},
 		// Try "arp -a" then.
-		&cmdARPDB{parse: parseArpAFunc, runcmd: rcArpA, ns: ns},
+		&cmdARPDB{parse: parseF, runcmd: rcArpA, ns: ns},
 		// Try "ip neigh" finally.
 		&cmdARPDB{parse: parseIPNeigh, runcmd: rcIPNeigh, ns: ns},
 	)
@@ -189,7 +189,7 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 
 // rcIPNeigh runs "ip neigh".
 func rcIPNeigh() (r io.Reader, err error) {
-	return rc("ip", "neigh")
+	return runCmd("ip", "neigh")
 }
 
 // parseIPNeigh parses the output of the "ip neigh" command on Linux.  The

internal/aghnet/arpdb_test.go

@@ -29,7 +29,7 @@ func (arp *TestARPDB) Neighbors() (ns []Neighbor) {
 	return arp.OnNeighbors()
 }
 
-func TestCompARPDB(t *testing.T) {
+func TestARPDBS(t *testing.T) {
 	knownIP := net.IP{1, 2, 3, 4}
 	knownMAC := net.HardwareAddr{0xAB, 0xCD, 0xEF, 0xAB, 0xCD, 0xEF}
 
@@ -52,7 +52,7 @@ func TestCompARPDB(t *testing.T) {
 	t.Run("begin_with_success", func(t *testing.T) {
 		t.Cleanup(clnp)
 
-		a := newCompARPDB(succDB, failDB)
+		a := newARPDBs(succDB, failDB)
 		err := a.Refresh()
 		require.NoError(t, err)
 
@@ -64,7 +64,7 @@ func TestCompARPDB(t *testing.T) {
 	t.Run("begin_with_fail", func(t *testing.T) {
 		t.Cleanup(clnp)
 
-		a := newCompARPDB(failDB, succDB)
+		a := newARPDBs(failDB, succDB)
 		err := a.Refresh()
 		require.NoError(t, err)
 
@@ -76,10 +76,9 @@ func TestCompARPDB(t *testing.T) {
 	t.Run("fail_only", func(t *testing.T) {
 		t.Cleanup(clnp)
 
-		wantMsg := `all implementations failed: 2 errors: ` +
-			`"refresh failed", "refresh failed"`
+		wantMsg := `each arpdb failed: 2 errors: "refresh failed", "refresh failed"`
 
-		a := newCompARPDB(failDB, failDB)
+		a := newARPDBs(failDB, failDB)
 		err := a.Refresh()
 		require.Error(t, err)
 
@@ -110,7 +109,7 @@ func TestCompARPDB(t *testing.T) {
 				return succDB.OnNeighbors()
 			},
 		}
-		a := newCompARPDB(unstableDB, succDB)
+		a := newARPDBs(unstableDB, succDB)
 
 		// Unstable ARPDB should refresh successfully.
 		err := a.Refresh()
@@ -135,7 +134,7 @@ func TestCompARPDB(t *testing.T) {
 	})
 
 	t.Run("empty", func(t *testing.T) {
-		a := newCompARPDB()
+		a := newARPDBs()
 		require.NoError(t, a.Refresh())
 
 		assert.Empty(t, a.Neighbors())

internal/aghnet/arpdb_unix.go

@@ -9,5 +9,5 @@ import (
 
 // rcArpA runs "arp -a".
 func rcArpA() (r io.Reader, err error) {
-	return rc("arp", "-a")
+	return runCmd("arp", "-a")
 }

internal/aghnet/arpdb_windows.go

@@ -24,7 +24,7 @@ func newARPDB() *cmdARPDB {
 
 // rcArpA runs "arp /a".
 func rcArpA() (r io.Reader, err error) {
-	return rc("arp", "/a")
+	return runCmd("arp", "/a")
 }
 
 // parseArpA parses the output of the "arp /a" command on Windows.  The expected

internal/aghos/os.go

@@ -52,11 +52,12 @@ func HaveAdminRights() (bool, error) {
 	return haveAdminRights()
 }
 
-// MaxCmdOutputSize is the maximum length of performed shell command output.
+// MaxCmdOutputSize is the maximum length of performed shell command output in
+// bytes.
 const MaxCmdOutputSize = 2 * 1024
 
 // RunCommand runs shell command.
-func RunCommand(command string, arguments ...string) (int, string, error) {
+func RunCommand(command string, arguments ...string) (code int, output string, err error) {
 	cmd := exec.Command(command, arguments...)
 	out, err := cmd.Output()
 	if len(out) > MaxCmdOutputSize {
@@ -66,7 +67,7 @@ func RunCommand(command string, arguments ...string) (int, string, error) {
 	if errors.As(err, new(*exec.ExitError)) {
 		return cmd.ProcessState.ExitCode(), string(out), nil
 	} else if err != nil {
-		return 1, "", fmt.Errorf("command %q failed: %w: %s", command, err, string(out))
+		return 1, "", fmt.Errorf("command %q failed: %w: %s", command, err, out)
 	}
 
 	return cmd.ProcessState.ExitCode(), string(out), nil

internal/home/clients.go

@@ -810,7 +810,7 @@ func (clients *clientsContainer) addFromHostsFile(hosts *netutil.IPMap) {
 // command.
 func (clients *clientsContainer) addFromSystemARP() {
 	if err := clients.arpdb.Refresh(); err != nil {
-		log.Debug("refreshing arp container: %s", err)
+		log.Error("refreshing arp container: %s", err)
 
 		clients.arpdb = aghnet.EmptyARPDB{}
 
@@ -837,7 +837,7 @@ func (clients *clientsContainer) addFromSystemARP() {
 		}
 	}
 
-	log.Debug("clients: added %d client aliases from ARP", added)
+	log.Debug("clients: added %d client aliases from arp neighborhood", added)
 }
 
 // updateFromDHCP adds the clients that have a non-empty hostname from the DHCP

internal/home/home.go

@@ -296,8 +296,7 @@ func setupConfig(args options) (err error) {
 	var arpdb aghnet.ARPDB
 	arpdb, err = aghnet.NewARPDB()
 	if err != nil {
-		// TODO(e.burkov):  !! is that ok?
-		log.Info("creating arp container: %s; using stub", err)
+		log.Info("warning: creating arpdb: %s; using stub", err)
 
 		arpdb = aghnet.EmptyARPDB{}
 	}