refactor: walk a subtree in steps, with its termination rules in one place

engine_harness_test.go

@@ -31,18 +31,28 @@ import (
 // engineTranscript is the ordered record of one engine scenario. A nil
 // transcript records nothing.
 type engineTranscript struct {
+	tb    testing.TB
 	start time.Time
 	lines []string
 }
 
-func newEngineTranscript() *engineTranscript {
-	return &engineTranscript{start: time.Now()}
+// maxTranscriptLines fails a scenario whose transcript grows past it, far
+// beyond the longest scenario, so that a change making the engine loop (a
+// regression or a mutant) fails at once instead of growing the transcript
+// until the test times out.
+const maxTranscriptLines = 5000
+
+func newEngineTranscript(tb testing.TB) *engineTranscript {
+	return &engineTranscript{tb: tb, start: time.Now()}
 }
 
 func (tr *engineTranscript) addf(format string, args ...any) {
 	if tr == nil {
 		return
 	}
+	if len(tr.lines) >= maxTranscriptLines {
+		tr.tb.Fatalf("transcript passed %d lines: the scenario does not end", maxTranscriptLines)
+	}
 	tr.lines = append(tr.lines, fmt.Sprintf("%-5s ", time.Since(tr.start))+fmt.Sprintf(format, args...))
 }
 

engine_requests_test.go

@@ -486,7 +486,7 @@ func TestEngineRequestCharacterization(t *testing.T) {
 // runEngineScenario runs sc in the calling synctest bubble and returns its
 // transcript.
 func runEngineScenario(t *testing.T, sc engineScenario) string {
-	tr := newEngineTranscript()
+	tr := newEngineTranscript(t)
 	c := newFakeTransport(tr, sc.agent)
 	var conn net.Conn
 	switch sc.shape {

engine_v3_test.go

@@ -679,7 +679,7 @@ func TestEngineV3Characterization(t *testing.T) {
 // transcript. The agent and the key oracle run without FIPS 140-only
 // enforcement: they stand for the other side, not the client.
 func runV3Scenario(t *testing.T, sc v3Scenario) string {
-	tr := newEngineTranscript()
+	tr := newEngineTranscript(t)
 	var agent *fakeV3Agent
 	fips140.WithoutEnforcement(func() { agent = newFakeV3Agent(tr, agentCreds, sysDescr) })
 	agent.script = sc.script

engine_walk_test.go

@@ -163,7 +163,10 @@ type walkScenario struct {
 	// stopAfter, when set, makes walkFn return an error on that value.
 	stopAfter int
 	// all runs WalkAll or BulkWalkAll instead of Walk or BulkWalk.
-	all      bool
+	all bool
+	// onValue, when set, runs in walkFn at the n-th value, as user code
+	// that changes the client during the walk.
+	onValue  func(x *GoSNMP, n int)
 	knownBug string
 }
 
@@ -236,6 +239,16 @@ func walkScenarios() map[string]walkScenario {
 		"leaf/last-object-v1": {
 			root: ".1.3.6.1.4.1.99.1.0", v1: true, knownBug: lastLeafBug,
 		},
+		// An agent echoing the root as the first answer: the root is taken
+		// for a leaf.
+		"leaf/root-echo": {
+			root: walkSystem, script: onWalkRequest(1, varbinds(SnmpPDU{Name: walkSystem, Type: Integer, Value: 1})),
+		},
+		"leaf/get-answer-in-subtree": {
+			root:     ".1.3.6.1.2.1.1.5.0",
+			script:   onWalkRequest(2, varbinds(SnmpPDU{Name: ".1.3.6.1.2.1.1.5.0.1", Type: OctetString, Value: []byte("inside")})),
+			knownBug: "after the leaf fallback the walk keeps sending Gets: an answer inside the subtree is walked and the walk gets the next name (net-snmp's snmpwalk makes one Get)",
+		},
 
 		// Answers a walk does not expect.
 		"empty-response": {
@@ -256,6 +269,11 @@ func walkScenarios() map[string]walkScenario {
 			root: walkIfDescr, setup: func(x *GoSNMP) { x.AppOpts = map[string]any{"c": true} },
 			script: onWalkRequest(3, varbinds(ifDescr(0))),
 		},
+		// The option counts by its key: a nil value turns the check off too.
+		"not-increasing/app-opts-c-nil": {
+			root: walkIfDescr, setup: func(x *GoSNMP) { x.AppOpts = map[string]any{"c": nil} },
+			script: onWalkRequest(3, varbinds(ifDescr(0))),
+		},
 		"not-increasing/app-opts-c-bulk": {
 			root: walkIfDescr, bulk: true,
 			setup: func(x *GoSNMP) {
@@ -284,6 +302,22 @@ func walkScenarios() map[string]walkScenario {
 			root: walkIfDescr, bulk: true, all: true, setup: func(x *GoSNMP) { x.MaxRepetitions = 1 },
 			script: func(n int, _, _ *SnmpPacket) bool { return n > 2 },
 		},
+		// A name that extends the root's last arc lies outside the subtree.
+		"sibling-arc": {
+			root:   walkIfDescr,
+			script: onWalkRequest(4, varbinds(SnmpPDU{Name: walkIfDescr + "0.1", Type: OctetString, Value: []byte("sibling")})),
+		},
+
+		// User code changing the client during a walk: the walk keeps the
+		// max-repetitions and the AppOpts it started with.
+		"user-code/walkfn-sets-max-repetitions": {
+			root: walkIfDescr, bulk: true, setup: func(x *GoSNMP) { x.MaxRepetitions = 1 },
+			onValue: func(x *GoSNMP, _ int) { x.MaxRepetitions = 2 },
+		},
+		"user-code/walkfn-sets-app-opts-c": {
+			root: walkIfDescr, script: onWalkRequest(3, varbinds(ifDescr(0))),
+			onValue: func(x *GoSNMP, _ int) { x.AppOpts = map[string]any{"c": true} },
+		},
 	}
 
 	// Every error status the walk names, and one it does not. NoSuchName
@@ -299,6 +333,13 @@ func walkScenarios() map[string]walkScenario {
 		root: walkSystem, script: onWalkRequest(2, errorStatus(InconsistentName+1)),
 		knownBug: "an error status the walk does not name is ignored and the varbinds walked",
 	}
+	scenarios["error-status/19-unknown-empty"] = walkScenario{
+		root: walkSystem,
+		script: onWalkRequest(2, func(_, out *SnmpPacket) {
+			out.Error, out.ErrorIndex, out.Variables = InconsistentName+1, 0, nil
+		}),
+		knownBug: "an empty response ends the walk with a nil error",
+	}
 	scenarios["error-status/bulk-too-big"] = walkScenario{
 		root: walkSystem, bulk: true, script: onWalkRequest(1, errorStatus(TooBig)),
 		knownBug: "an error status ends the walk with a nil error and the values so far",
@@ -328,7 +369,7 @@ func TestEngineWalkCharacterization(t *testing.T) {
 // runWalkScenario runs sc in the calling synctest bubble and returns its
 // transcript.
 func runWalkScenario(t *testing.T, sc walkScenario) string {
-	tr := newEngineTranscript()
+	tr := newEngineTranscript(t)
 	agent := &mibAgent{tr: tr, mib: walkMIB, script: sc.script}
 	version := Version2c
 	if sc.v1 {
@@ -357,6 +398,9 @@ func runWalkScenario(t *testing.T, sc walkScenario) string {
 			walkFn := func(vb SnmpPDU) error {
 				values++
 				tr.addf("walkFn: %s", describeVarbinds([]SnmpPDU{vb}))
+				if sc.onValue != nil {
+					sc.onValue(x, values)
+				}
 				if values == sc.stopAfter {
 					return errWalkFn
 				}
@@ -381,3 +425,31 @@ func runWalkScenario(t *testing.T, sc walkScenario) string {
 	}
 	return strings.Join(tr.lines, "\n")
 }
+
+// BenchmarkWalk runs WalkAll and BulkWalkAll, as the Netdata Agent's SNMP
+// collector does, over a 100-row table column on the in-memory agent.
+func BenchmarkWalk(b *testing.B) {
+	const root = ".1.3.6.1.4.1.99999.1.1.1.1.2.3.4"
+	mib := make([]SnmpPDU, 0, 101)
+	for i := range 100 {
+		mib = append(mib, SnmpPDU{Name: fmt.Sprintf("%s.%d", root, i+1), Type: Counter64, Value: uint64(i)})
+	}
+	mib = append(mib, SnmpPDU{Name: ".1.3.6.1.4.1.99999.1.1.1.1.2.3.5.1", Type: Integer, Value: 1})
+
+	for name, walkAll := range map[string]func(x *GoSNMP) ([]SnmpPDU, error){
+		"WalkAll":     func(x *GoSNMP) ([]SnmpPDU, error) { return x.WalkAll(root) },
+		"BulkWalkAll": func(x *GoSNMP) ([]SnmpPDU, error) { return x.BulkWalkAll(root) },
+	} {
+		b.Run(name, func(b *testing.B) {
+			agent := &mibAgent{mib: mib}
+			x := newEngineClient(b, nil, Version2c, newFakeTransport(nil, agent.handle))
+			b.ReportAllocs()
+			for b.Loop() {
+				vbs, err := walkAll(x)
+				if err != nil || len(vbs) != 100 {
+					b.Fatalf("%d values, error %v", len(vbs), err)
+				}
+			}
+		})
+	}
+}

gosnmp.go

@@ -368,47 +368,6 @@ func (x *GoSNMP) SnmpDecodePacket(resp []byte) (*SnmpPacket, error) {
 	return result, nil
 }
 
-//
-// SNMP Walk functions - Analogous to net-snmp's snmpwalk commands
-//
-
-// WalkFunc is the type of the function called for each data unit visited
-// by the Walk function.  If an error is returned processing stops.
-type WalkFunc func(dataUnit SnmpPDU) error
-
-// BulkWalk retrieves a subtree of values using GETBULK. As the tree is
-// walked walkFn is called for each new value. The function immediately returns
-// an error if either there is an underlaying SNMP error (e.g. GetBulk fails),
-// or if walkFn returns an error.
-func (x *GoSNMP) BulkWalk(rootOid string, walkFn WalkFunc) error {
-	return x.walk(GetBulkRequest, rootOid, walkFn)
-}
-
-// BulkWalkAll is similar to BulkWalk but returns a filled array of all values
-// rather than using a callback function to stream results. Caution: if you
-// have set x.AppOpts to 'c', BulkWalkAll may loop indefinitely and cause an
-// Out Of Memory - use BulkWalk instead.
-func (x *GoSNMP) BulkWalkAll(rootOid string) (results []SnmpPDU, err error) {
-	return x.walkAll(GetBulkRequest, rootOid)
-}
-
-// Walk retrieves a subtree of values using GETNEXT - a request is made for each
-// value, unlike BulkWalk which does this operation in batches. As the tree is
-// walked walkFn is called for each new value. The function immediately returns
-// an error if either there is an underlaying SNMP error (e.g. GetNext fails),
-// or if walkFn returns an error.
-func (x *GoSNMP) Walk(rootOid string, walkFn WalkFunc) error {
-	return x.walk(GetNextRequest, rootOid, walkFn)
-}
-
-// WalkAll is similar to Walk but returns a filled array of all values rather
-// than using a callback function to stream results. Caution: if you have set
-// x.AppOpts to 'c', WalkAll may loop indefinitely and cause an Out Of Memory -
-// use Walk instead.
-func (x *GoSNMP) WalkAll(rootOid string) (results []SnmpPDU, err error) {
-	return x.walkAll(GetNextRequest, rootOid)
-}
-
 //
 // Public Functions (helpers) - in alphabetical order
 //