Conversation
|
|
lextm
left a comment
lextm
left a comment
There was a problem hiding this comment.
Default values should be OctetString.Empty instead of null. And then you can annotate the parameters as non-nullable.
|
Hello @lextm, Do you need another fix / commit ? Thanks |
There was a problem hiding this comment.
Pull request overview
This PR adds support for specifying an SNMPv3 contextName during discovery so that discovery requests can target devices that require a non-empty context name.
Changes:
- Added
Discover/DiscoverAsyncoverloads (net471 + net6) that accept acontextNameand pass it into v3 discovery messages. - Added
Messenger.GetNextDiscoveryoverload that acceptscontextName. - Added new
Discoveryconstructors that populateScope.ContextNameand updated v3 discovery creation to use them.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| SharpSnmpLib/Messaging/Net6Discoverer.cs | Adds overloads to pass contextName into v3 discovery for .NET 6+ discoverer. |
| SharpSnmpLib/Messaging/Discoverer.cs | Adds overloads to pass contextName into v3 discovery for non-.NET6 discoverer. |
| SharpSnmpLib/Messaging/Messenger.cs | Adds GetNextDiscovery overload that accepts contextName (and changes default to call it). |
| SharpSnmpLib/Messaging/Discovery.cs | Adds constructors supporting contextName and modifies response handling logic in GetResponse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <param name="interval">The discovering time interval, in milliseconds.</param> | ||
| /// <param name="contextName">The optional Context Name.</param> | ||
| /// <remarks><paramref name="broadcastAddress"/> must be configured to a valid multicast address when IPv6 is used. For example, "[ff02::1]:161"</remarks> | ||
| public void Discover(VersionCode version, IPEndPoint broadcastAddress, OctetString community, int interval, OctetString contextName) | ||
| { |
There was a problem hiding this comment.
The new Discover overload takes contextName as a required non-nullable argument, but the XML docs call it optional and it’s only meaningful for v3. Consider making it nullable/optional (and defaulting null to OctetString.Empty) to avoid surprising ArgumentNullExceptions for callers who treat it as optional.
| /// <param name="community">The community.</param> | ||
| /// <param name="token">The cancellation token.</param> | ||
| /// <param name="contextName">The optional context name.</param> | ||
| /// <remarks><paramref name="broadcastAddress"/> must be an IPv4 address. IPv6 is not yet supported here.</remarks> | ||
| public void Discover(VersionCode version, IPEndPoint broadcastAddress, OctetString? community, CancellationToken token, OctetString contextName) | ||
| { |
There was a problem hiding this comment.
The new Discover/DiscoverAsync overloads take contextName as a required non-nullable argument, but docs describe it as optional. Consider making contextName nullable/optional (and defaulting null to OctetString.Empty) so callers don’t get unexpected ArgumentNullExceptions when Scope is constructed.
| public Discovery(int messageId, int requestId, int maxMessageSize, OctetString contextName) | ||
| { | ||
| _discovery = new GetRequestMessage( | ||
| VersionCode.V3, | ||
| new Header( | ||
| new Integer32(messageId), | ||
| new Integer32(maxMessageSize), | ||
| Levels.Reportable), | ||
| DefaultSecurityParameters, | ||
| new Scope( | ||
| OctetString.Empty, | ||
| contextName, | ||
| new GetRequestPdu(requestId, new List<Variable>())), | ||
| DefaultPrivacyProvider.DefaultPair, | ||
| null); |
There was a problem hiding this comment.
New contextName support in Discovery constructors isn’t covered by unit tests. Since there are existing messaging/parse tests, consider adding a test that builds a Discovery with a non-empty contextName and asserts the serialized bytes (or parsed Scope.ContextName) contains that contextName.
| /// Returns a new discovery request. | ||
| /// </summary> | ||
| /// <param name="type">Message type.</param> | ||
| /// <param name="contextName">The optional context name.</param> |
There was a problem hiding this comment.
XML doc for GetNextDiscovery(SnmpType type) includes a tag, but the method has no such parameter. This will produce incorrect generated docs/warnings; remove that param tag (keep it only on the overload that actually accepts contextName).
| /// <param name="contextName">The optional context name.</param> |
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="Discovery"/> class. | ||
| /// </summary> | ||
| /// <param name="requestId">The request id.</param> | ||
| /// <param name="messageId">The message id.</param> | ||
| /// <param name="maxMessageSize">The max size of message.</param> | ||
| /// <param name="contextName">The optional Context Name.</param> | ||
| public Discovery(int messageId, int requestId, int maxMessageSize, OctetString contextName) | ||
| { |
There was a problem hiding this comment.
The new Discovery(.., OctetString contextName) constructor documents parameters as requestId then messageId, but the signature is (messageId, requestId, ...). Please fix the XML doc param names/order so they match the actual arguments.
| var response = _discovery.GetResponse(timeout, receiver, Empty, socket); | ||
|
|
||
| try | ||
| { | ||
| return (ReportMessage)response; | ||
| } | ||
| catch | ||
| { | ||
| //The cast was not possible with a system that needed contextname=public | ||
| return new ReportMessage(response.Version, response.Header, response.Parameters, response.Scope, response.Privacy, response?.ToBytes()); | ||
| } |
There was a problem hiding this comment.
In the fallback path, ReportMessage is constructed with response.ToBytes() passed as the length argument. That parameter represents ASN.1 length bytes (see StreamExtension.AppendBytes: it writes the provided length directly), so passing the full message bytes will produce an invalid encoding. If you need to handle non-Report responses, avoid reusing ToBytes() as the length; instead compute a new message (pass null for length) and gate this path using a type check (e.g., response is ReportMessage).
| /// <param name="contextName">The optional Context Name.</param> | ||
| public Discovery(int messageId, int requestId, int maxMessageSize, OctetString contextName) | ||
| { | ||
| _discovery = new GetRequestMessage( | ||
| VersionCode.V3, | ||
| new Header( | ||
| new Integer32(messageId), | ||
| new Integer32(maxMessageSize), | ||
| Levels.Reportable), | ||
| DefaultSecurityParameters, | ||
| new Scope( | ||
| OctetString.Empty, | ||
| contextName, | ||
| new GetRequestPdu(requestId, new List<Variable>())), | ||
| DefaultPrivacyProvider.DefaultPair, | ||
| null); |
There was a problem hiding this comment.
contextName is described as optional but the parameter is non-nullable and is passed directly into Scope, which throws if contextName is null. Consider accepting OctetString? (or validating and defaulting null to OctetString.Empty) so callers don’t hit an unexpected ArgumentNullException inside Scope.
In V3, to connect to some devices, we need to initialize the scope contextName from the discovery