markabrahams / node-net-snmp

JavaScript implementation of the Simple Network Management Protocol (SNMP)
206 stars 97 forks source link

Use MIB default values for scalars at registration. #236

Open abutcher-gh opened 1 year ago

abutcher-gh commented 1 year ago

If a default is given in the MIB, resolve it to the appropriate type and set it on the scalarDefinition. Enumeration types are handled; both enumerator name and integral value are supported. JSON.parse is used to resolve quoted strings and numbers to appropriate JS types.

markabrahams commented 1 year ago

Hi @abutcher-gh - thanks for the PR!

The defVal in the provider definition aims to capture the intent of the DEFVAL MIB clause in the SNMP RFCs. RFC 2578 Section 4.9 states:

The DEFVAL clause, which need not be present, defines an acceptable default value which may be used at the discretion of an agent when an object instance is created. That is, the value is a "hint" to implementors.

So the use of DEFVAL is "at the discretion of an agent" and to be used "when an object instance is created". It does go on to say:

Note that with this definition of the DEFVAL clause, it is appropriate to use it for any columnar object of a read-create table. It is also permitted to use it for scalar objects dynamically created by an agent, or for columnar objects of a read-write table dynamically created by an agent.

And it is silent on any role that DEFVAL may or may not have in MIB initialisation.

As such, I'm thinking that attaching another behaviour to the library's current "defVal" behaviour, may introduce an unexpected, undesirable and breaking change for some use cases.

Perhaps if we add a boolean agent option (also passed to a new MIB options argument on snmp.createMib() ) called addScalarDefaultsOnRegistration (bit of a mouthful I know!), this would provide options for both cases. Probably defaulting to the current behaviour so as not to introduce a possible breaking change.

@derrell - I know you worked on the defVal implementation, so would be interested to hear your thoughts here.

Also, the first part of your PR suggests that defVal for enumerated types aren't currently working - have you found that to be the case? If so, maybe splitting PR into a bug for that and another for the automatic scalar default creation feature would be good.

Finally, I think it would be good to extract your new defVal conversion for scalars into a re-usable function, as any defVal conversion logic in place for scalars should be similarly called for table column default values as well.

derrell commented 1 year ago

@markabrahams That work was quite a while ago! It seems, though, that I may have missed scaler defaults, and I would prefer that "hints" be honored. I endorse "fixing" this so that scalars' defaults are set. I agree, though, that it is a non-backward-compatible change, so either do as you suggested and add an option (I don't mind the mouthful name)... or increment the major version number. I would opt for the incrementing the major version number and not adding a new option, as honoring default specifications would seem the more expected capability.