Pull request 2271: AGDNS-2374-slog-arpdb

Squashed commit of the following: commit 355136e Author: Stanislav Chzhen <[email protected]> Date: Tue Aug 27 18:09:51 2024 +0300 arpdb: imp docs commit 2738383 Author: Stanislav Chzhen <[email protected]> Date: Mon Aug 26 19:11:10 2024 +0300 all: slog arpdb
annguyen0 · Aug 27, 2024 · 0b8bf13 · 0b8bf13
1 parent 738958d
commit 0b8bf13
Show file tree

Hide file tree

Showing 8 changed files with 110 additions and 142 deletions.
diff --git a/internal/arpdb/arpdb.go b/internal/arpdb/arpdb.go
@@ -5,14 +5,15 @@ import (
 	"bufio"
 	"bytes"
 	"fmt"
+	"log/slog"
 	"net"
 	"net/netip"
 	"slices"
 	"sync"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
 	"github.com/AdguardTeam/golibs/errors"
-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/osutil"
 )
@@ -38,8 +39,8 @@ type Interface interface {
 }
 
 // New returns the [Interface] properly initialized for the OS.
-func New() (arp Interface) {
-	return newARPDB()
+func New(logger *slog.Logger) (arp Interface) {
+	return newARPDB(logger)
 }
 
 // Empty is the [Interface] implementation that does nothing.
@@ -69,6 +70,30 @@ type Neighbor struct {
 	MAC net.HardwareAddr
 }
 
+// newNeighbor returns the new initialized [Neighbor] by parsing string
+// representations of IP and MAC addresses.
+func newNeighbor(host, ipStr, macStr string) (n *Neighbor, err error) {
+	defer func() { err = errors.Annotate(err, "getting arp neighbor: %w") }()
+
+	ip, err := netip.ParseAddr(ipStr)
+	if err != nil {
+		// Don't wrap the error, as it will get annotated.
+		return nil, err
+	}
+
+	mac, err := net.ParseMAC(macStr)
+	if err != nil {
+		// Don't wrap the error, as it will get annotated.
+		return nil, err
+	}
+
+	return &Neighbor{
+		Name: host,
+		IP:   ip,
+		MAC:  mac,
+	}, nil
+}
+
 // Clone returns the deep copy of n.
 func (n Neighbor) Clone() (clone Neighbor) {
 	return Neighbor{
@@ -80,10 +105,10 @@ func (n Neighbor) Clone() (clone Neighbor) {
 
 // validatedHostname returns h if it's a valid hostname, or an empty string
 // otherwise, logging the validation error.
-func validatedHostname(h string) (host string) {
+func validatedHostname(logger *slog.Logger, h string) (host string) {
 	err := netutil.ValidateHostname(h)
 	if err != nil {
-		log.Debug("arpdb: parsing arp output: host: %s", err)
+		logger.Debug("parsing host of arp output", slogutil.KeyError, err)
 
 		return ""
 	}
@@ -132,15 +157,18 @@ func (ns *neighs) reset(with []Neighbor) {
 // parseNeighsFunc 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 parseNeighsFunc func(sc *bufio.Scanner, lenHint int) (ns []Neighbor)
+//
+// TODO(s.chzhen):  Return []*Neighbor instead.
+type parseNeighsFunc func(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor)
 
 // cmdARPDB is the implementation of the [Interface] that uses command line to
 // retrieve data.
 type cmdARPDB struct {
-	parse parseNeighsFunc
-	ns    *neighs
-	cmd   string
-	args  []string
+	logger *slog.Logger
+	parse  parseNeighsFunc
+	ns     *neighs
+	cmd    string
+	args   []string
 }
 
 // type check
@@ -158,7 +186,7 @@ func (arp *cmdARPDB) Refresh() (err error) {
 	}
 
 	sc := bufio.NewScanner(bytes.NewReader(out))
-	ns := arp.parse(sc, arp.ns.len())
+	ns := arp.parse(arp.logger, sc, arp.ns.len())
 	if err = sc.Err(); err != nil {
 		// TODO(e.burkov):  This error seems unreachable.  Investigate.
 		return fmt.Errorf("scanning the output: %w", err)

diff --git a/internal/arpdb/arpdb_bsd.go b/internal/arpdb/arpdb_bsd.go
@@ -4,17 +4,17 @@ package arpdb
 
 import (
 	"bufio"
-	"net"
-	"net/netip"
+	"log/slog"
 	"strings"
 	"sync"
 
-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 )
 
-func newARPDB() (arp *cmdARPDB) {
+func newARPDB(logger *slog.Logger) (arp *cmdARPDB) {
 	return &cmdARPDB{
-		parse: parseArpA,
+		logger: logger,
+		parse:  parseArpA,
 		ns: &neighs{
 			mu: &sync.RWMutex{},
 			ns: make([]Neighbor, 0),
@@ -33,7 +33,7 @@ func newARPDB() (arp *cmdARPDB) {
 // The expected input format:
 //
 //	host.name (192.168.0.1) at ff:ff:ff:ff:ff:ff on en0 ifscope [ethernet]
-func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpA(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	ns = make([]Neighbor, 0, lenHint)
 	for sc.Scan() {
 		ln := sc.Text()
@@ -48,26 +48,15 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}
 
-		ip, err := netip.ParseAddr(ipStr[1 : len(ipStr)-1])
+		host := validatedHostname(logger, fields[0])
+		n, err := newNeighbor(host, ipStr[1:len(ipStr)-1], fields[3])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)
 
 			continue
 		}
 
-		hwStr := fields[3]
-		mac, err := net.ParseMAC(hwStr)
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		}
-
-		ns = append(ns, Neighbor{
-			IP:   ip,
-			MAC:  mac,
-			Name: validatedHostname(fields[0]),
-		})
+		ns = append(ns, *n)
 	}
 
 	return ns

diff --git a/internal/arpdb/arpdb_internal_test.go b/internal/arpdb/arpdb_internal_test.go
@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/AdguardTeam/golibs/errors"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 	"github.com/AdguardTeam/golibs/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -61,7 +62,7 @@ func (s mapShell) RunCmd(cmd string, args ...string) (code int, out []byte, err
 
 func Test_New(t *testing.T) {
 	var a Interface
-	require.NotPanics(t, func() { a = New() })
+	require.NotPanics(t, func() { a = New(slogutil.NewDiscardLogger()) })
 
 	assert.NotNil(t, a)
 }
@@ -201,8 +202,9 @@ func Test_NewARPDBs(t *testing.T) {
 
 func TestCmdARPDB_arpa(t *testing.T) {
 	a := &cmdARPDB{
-		cmd:   "cmd",
-		parse: parseArpA,
+		logger: slogutil.NewDiscardLogger(),
+		cmd:    "cmd",
+		parse:  parseArpA,
 		ns: &neighs{
 			mu: &sync.RWMutex{},
 			ns: make([]Neighbor, 0),

diff --git a/internal/arpdb/arpdb_linux.go b/internal/arpdb/arpdb_linux.go
@@ -6,17 +6,18 @@ import (
 	"bufio"
 	"fmt"
 	"io/fs"
+	"log/slog"
 	"net"
 	"net/netip"
 	"strings"
 	"sync"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
-	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
 	"github.com/AdguardTeam/golibs/stringutil"
 )
 
-func newARPDB() (arp *arpdbs) {
+func newARPDB(logger *slog.Logger) (arp *arpdbs) {
 	// Use the common storage among the implementations.
 	ns := &neighs{
 		mu: &sync.RWMutex{},
@@ -39,9 +40,10 @@ func newARPDB() (arp *arpdbs) {
 		},
 		// Then, try "arp -a -n".
 		&cmdARPDB{
-			parse: parseF,
-			ns:    ns,
-			cmd:   "arp",
+			logger: logger,
+			parse:  parseF,
+			ns:     ns,
+			cmd:    "arp",
 			// Use -n flag to avoid resolving the hostnames of the neighbors.
 			// By default ARP attempts to resolve the hostnames via DNS.  See
 			// man 8 arp.
@@ -51,10 +53,11 @@ func newARPDB() (arp *arpdbs) {
 		},
 		// Finally, try "ip neigh".
 		&cmdARPDB{
-			parse: parseIPNeigh,
-			ns:    ns,
-			cmd:   "ip",
-			args:  []string{"neigh"},
+			logger: logger,
+			parse:  parseIPNeigh,
+			ns:     ns,
+			cmd:    "ip",
+			args:   []string{"neigh"},
 		},
 	)
 }
@@ -131,7 +134,7 @@ func (arp *fsysARPDB) Neighbors() (ns []Neighbor) {
 //
 //	IP address     HW type  Flags  HW address         Mask  Device
 //	192.168.11.98  0x1      0x2    5a:92:df:a9:7e:28  *     wan
-func parseArpAWrt(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpAWrt(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	if !sc.Scan() {
 		// Skip the header.
 		return
@@ -146,25 +149,14 @@ func parseArpAWrt(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}
 
-		ip, err := netip.ParseAddr(fields[0])
+		n, err := newNeighbor("", fields[0], fields[3])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)
 
 			continue
 		}
 
-		hwStr := fields[3]
-		mac, err := net.ParseMAC(hwStr)
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
-
-			continue
-		}
-
-		ns = append(ns, Neighbor{
-			IP:  ip,
-			MAC: mac,
-		})
+		ns = append(ns, *n)
 	}
 
 	return ns
@@ -174,7 +166,7 @@ func parseArpAWrt(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 // expected input format:
 //
 //	hostname (192.168.1.1) at ab:cd:ef:ab:cd:ef [ether] on enp0s3
-func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseArpA(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	ns = make([]Neighbor, 0, lenHint)
 	for sc.Scan() {
 		ln := sc.Text()
@@ -189,26 +181,15 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}
 
-		ip, err := netip.ParseAddr(ipStr[1 : len(ipStr)-1])
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
-
-			continue
-		}
-
-		hwStr := fields[3]
-		mac, err := net.ParseMAC(hwStr)
+		host := validatedHostname(logger, fields[0])
+		n, err := newNeighbor(host, ipStr[1:len(ipStr)-1], fields[3])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)
 
 			continue
 		}
 
-		ns = append(ns, Neighbor{
-			IP:   ip,
-			MAC:  mac,
-			Name: validatedHostname(fields[0]),
-		})
+		ns = append(ns, *n)
 	}
 
 	return ns
@@ -218,7 +199,7 @@ func parseArpA(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 // expected input format:
 //
 //	192.168.1.1 dev enp0s3 lladdr ab:cd:ef:ab:cd:ef REACHABLE
-func parseIPNeigh(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
+func parseIPNeigh(logger *slog.Logger, sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 	ns = make([]Neighbor, 0, lenHint)
 	for sc.Scan() {
 		ln := sc.Text()
@@ -228,27 +209,14 @@ func parseIPNeigh(sc *bufio.Scanner, lenHint int) (ns []Neighbor) {
 			continue
 		}
 
-		n := Neighbor{}
-
-		ip, err := netip.ParseAddr(fields[0])
-		if err != nil {
-			log.Debug("arpdb: parsing arp output: ip: %s", err)
-
-			continue
-		} else {
-			n.IP = ip
-		}
-
-		mac, err := net.ParseMAC(fields[4])
+		n, err := newNeighbor("", fields[0], fields[4])
 		if err != nil {
-			log.Debug("arpdb: parsing arp output: mac: %s", err)
+			logger.Debug("parsing arp output", "line", ln, slogutil.KeyError, err)
 
 			continue
-		} else {
-			n.MAC = mac
 		}
 
-		ns = append(ns, n)
+		ns = append(ns, *n)
 	}
 
 	return ns