Repository navigation
refactor: one request builder and one request and msg ID generator - #42
Merged
Merged
Conversation
ilyam8
added this pull request to stack #38
October 9, 2026 15:18
ilyam8
changed the title
ilyam8
force-pushed
the
engine-requests
branch
2 times, most recently
from
October 11, 2026 09:46
1e0f37c to
08ee3ea
Compare
Mutation probes on the request and ID code found four behaviors no test observed: GetBulk checks for SNMPv1 before MaxOids; Connect seeds both the request and the msg ID from one random value and reseeds them from the same value on every Connect (known bug: it discards SetRequestID and SetMsgID); SetMsgID moves the msg ID; the msg ID wraps to 0 after 2147483647. TestGetBulkChecks and TestRequestAndMsgIDs pin them through the public API (SnmpEncodePacket and SnmpDecodePacket read the IDs of the encoded messages); both pass on the current code.
The OID operations and the ID handling were copied: Get, GetNext and GetBulk each checked MaxOids and built their Null varbinds, and sendOneRequest and SnmpEncodePacket each advanced and masked both IDs, while Connect and the setters wrote the same fields without atomics. - request.go: the operations and the packet builders; oidRequest does the MaxOids check and the OID to varbind conversion for Get, GetNext and GetBulk (GetBulk still checks for SNMPv1 first; Set is unchanged, its first-varbind type check and its nil panic included). - ids.go: the request and msg ID counters as atomic.Uint32, advanced and masked to 31 bits in nextRequestID and nextMsgID, seeded by Connect from the client's random value, set by SetRequestID and SetMsgID. No behavior change: every golden and the API test are unchanged.
ilyam8
force-pushed
the
engine-requests
branch
from
October 11, 2026 09:52
08ee3ea to
ce4c8c5
Compare
vkalintiris
approved these changes
Oct 11, 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
First restructuring step of the request engine: one request builder and one request and msg ID generator. No behavior or exported API change.
request.go:Get,GetNext,GetBulk,Setand the packet builders.oidRequestdoes theMaxOidscheck and the OID to Null-varbind conversion once instead of three times.GetBulkstill checks for SNMPv1 first;Setis unchanged (its first-varbind type check and itsSet(nil)panic included).ids.go: the request and msg ID counters asatomic.Uint32, advanced and masked to 31 bits innextRequestIDandnextMsgID(used bysendOneRequestandSnmpEncodePacket, which each had a copy), seeded byConnect, set bySetRequestIDandSetMsgID.Connectand the setters no longer write the fields without atomics.The first commit pins four behaviors no test observed, found by mutation probes, through the public API (
SnmpEncodePacket/SnmpDecodePacketread the IDs of the encoded messages):GetBulkchecks for SNMPv1 beforeMaxOids;Connectseeds both IDs from one random value and reseeds them on everyConnect(known bug: it discardsSetRequestID/SetMsgID);SetMsgIDmoves the msg ID; the msg ID wraps to 0 after 2147483647. Both pass on the previous code.Testing
go test ./...on darwin/arm64 andGOARCH=386;-race; the FIPS 140-only tests; golangci-lint v2.14.0 (incl.end2end): 0 issues.requests,v3,walk, codec) and the API test unchanged.(x & m) + 1 & m == (x + 1) & m).BenchmarkSendOneRequest, interleaved A/B, 6 rounds: time unchanged (p = 0.589), B/op and allocs/op identical.