Repository navigation
refactor: tidy the request engine and API - #56
Merged
Merged
Conversation
settleReply took send's error and handed it back on four of its five paths, so the error a request ended with was decided inside a helper about storing engine parameters. It now returns only the store's error (for a reply other than an error Report), and send decides at each of its two call sites: the store error first, then the resync, otherwise its own error. Results are the same on every path.
When the unconnected UDP socket cannot be opened (here on an address no local interface has), Connect returns the listen error and leaves Conn holding a nil *net.UDPConn, which is not nil: a known bug.
netConnect set its error and local address in switch branches and dialed after the switch with whatever they held. Each transport now resolves its own local address and returns: dial connects with it (nil for transports other than UDP and TCP, as before), and listenUnconnectedUDP opens the unconnected socket, assigning the listen result to Conn as before.
validateParameters filled in defaults (transport, MaxOids, Context), rejected a negative MaxOids and, for SNMPv3, set Reportable, checked the security parameters and seeded the salt counter: validation was one of four jobs. It is prepare now, documented with all four and with its callers, which run it at every call. SnmpEncodePacket and SnmpDecodePacket declare their errors where they are set and return early; their results are unchanged.
- Set tests the first varbind's type as a case list and sends after it, instead of assigning the packet inside a one-case switch; the stale "TODO test Gauge32" goes (a Gauge32 Set is pinned). - MkSnmpPacket holds what mkSnmpPacket did, now documented; the unexported copy is gone. - The top-bit mask of request IDs, msgIDs and max-repetitions is math.MaxInt32 at every site (INTEGER (0..2147483647)); the Handler's MaxRepetitions getter no longer masks a field only its masking setter writes. - oidCompare compares components with cmp.Compare.
…er's mask - A TCP connection is made from the configured local address, port included (on loopback only the port tells it apart). - SnmpEncodePacket returns an empty, non-nil message with its errors. - A Handler keeps max-repetitions with its top bit cleared. Each passes on the code before the engine and API tidy-ups.
The zero kind on a failed store told send's resync test to stop when the store error shared the request's error slot. send now checks the store error first and never reads the kind with it, so settleReply returns the reply's kind on every path.
ilyam8
added this pull request to stack #53
October 13, 2026 10:36
ilyam8
marked this pull request as ready for review
October 13, 2026 10:36
stelfrag
approved these changes
Oct 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 6 (guardrails), engine and API tidy-ups. No behavior change, no exported API change.
settleReply. It tooksend's error and handed it back on four of its five paths, so the error a request endedwith was decided inside a helper about storing engine parameters. It now returns only the store's error, and
senddecides at each call site: the store error first, then the resync, otherwise its own error.
netConnect. Each transport resolves its own local address and returns, throughdialorlistenUnconnectedUDP, instead of branches that set shared variables for a dial after the switch.prepare.validateParametersfilled in defaults, rejected a negative MaxOids and, for SNMPv3, set Reportable,checked the security parameters and seeded the salt counter; it is
preparenow, documented with all four jobs.SnmpEncodePacketandSnmpDecodePacketdeclare their errors where they are set.Settests the first varbind's type as a case list and sends after it;MkSnmpPacketholds thebody of the unexported copy it wrapped, now documented; the top-bit mask of request IDs, msgIDs and max-repetitions
is
math.MaxInt32at every site, and the Handler's getter no longer masks a field only its masking setter writes;oidCompareusescmp.Compare.Validation
Connholding a nil*net.UDPConn(a known bug, kept); a TCP connection uses the configured local port;SnmpEncodePacketreturns anempty, non-nil message with its errors; a Handler clears the top bit of max-repetitions.
encode error output, the Handler mask, and the failed-listen state).
BenchmarkGetandBenchmarkGetV3(6 interleaved rounds against the previous PR's head): time within noise,bytes and allocations identical.
go test -race, FIPS 140-only, golangci-lint, cross-builds, govulncheck: pass locally.