[SNMP4J] Unnecessary object creation in MessageDispatcherImpl

[Frank Fock]

Hi Chris,

Thank you for your comments!

Am 26.11.2013 04:58, schrieb Chris Janicki:
> Hi,
>
> I've been tinkering with SNMP4J 2.  Nice work!  I have three miscellaneous observations, if you're interested:
>
> 1) Regarding SNMP4J 2, org.snmp4j.MessageDispatcherImpl line# 465 creates: new OctetString(securityName),  But that OctetString is already available in target.getSecurityName(), as seen at line# 406.  Unless I'm missing some threading reason why you want a new instance, could it be a tiny bit better to reuse the object?
This has been done intentionally. In a good world, you would have been 
right and we could save the extra
object creation. But users often change the Target or PDU objects while 
a request is being processed.
While a request is being sent out, this must be strictly avoided by the 
caller - that's clear.
But here we are creating a TransportStateReference which lives until a 
response for request is received
or a timeout occurs. Here I think it is more safe to create a new 
(unchangable) instance.
> 2) Unless it's an RFC requirement, I don't really understand why the Target requires the SecurityLevel?... It seems redundant since the SecurityName points to a User with certain security credentials.  Would you ever specify a SecurityLevel that is different than what the User's credentials (authProtocol/authPassword and privProtocol/privPassword) contains?  Would it be an improvement to simply infer the SecurityLevel by whether or not the User has non-null auth and/or priv settings defined?
Yes, the SNMP standard requires a security level. You can (must be able 
to) use
an user that has privacy defined with no privacy, for example. With USM 
alone, one might be able
to deduce the (max) security level easily, but with security protocols 
as TLS it is not possible.

The architecture defined by the IETF requires a separate security level 
and it would not be wise
to change. Otherwise, future changes in the standards would break the 
existing SNMP4J API
which would lead to higher efforts for the API users.

If you want to do this mapping, you will have to do so in your 
application code.

>
> 3) In the javadoc of CommunityTarget.setCommunity(), it would be nice to explain that this is just a convenience method for setSecurityName().  Also, getCommunity()'s javadoc currently states that the reply is never 'null', but it can be if the default CommunityTarget() constructor is used, or if the superclass's setSecurityName() is used to set it to null.  (But please don't change AbstractTarget.setSecurityName() to reject 'null', because I'm currently using that null as a "flag" to infer that my app should simply reference the securityName of related incoming trap rather than use some pre-set value assigned to the Target.)
I improved the JavaDoc. Thank you for pointing this out.

Best regards,
Frank

-- 
---
AGENT++
Maximilian-Kolbe-Str. 10
73257 Koengen, Germany
https://agentpp.com
Phone: +49 7024 8688230
Fax:   +49 7024 8688231