Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactoring of the constant? #17

Closed
tehnerd opened this issue Jul 16, 2014 · 6 comments
Closed

refactoring of the constant? #17

tehnerd opened this issue Jul 16, 2014 · 6 comments
Assignees

Comments

@tehnerd
Copy link

tehnerd commented Jul 16, 2014

do you consider to refactor this part of the code

const (
    // maxOids is the maximum number of oids allowed in a Get()
    maxOids = 60

    // Base OID for MIB-2 defined SNMP variables
    baseOid = ".1.3.6.1.2.1"

    // Java SNMP uses 50, snmp-net uses 10
    defaultMaxRepetitions = 50

    // TODO comment
    defaultNonRepeaters = 0
)

by moving it to the Default for example.
the reason is:
a lot of networking hardware uses oids from the private space (.1.3.6.1.4...)
(.1.3.6.1.2 is IETF only)
as well as i have found a problem with defaultMaxRepetitions equals to 50 size on some of the networking hardware (so IMO it's better to be configurable instead of hardcoding)

@virtuallynathan
Copy link
Contributor

Hey @tehnerd,

I think these were set by the original author as sensible defaults, they can of course be changed. If you, and the community at large feel these aren't actually sensible, then I suppose it makes sense to change them. Thoughts @soniah & @codedance?

@soniah
Copy link
Collaborator

soniah commented Jul 17, 2014

I'll have to look at the code in detail...

Hey @tehnerd,

I think these were set by the original author as sensible defaults, they can of course be changed. If you, and the community at large feel these aren't actually sensible, then I suppose it makes sense to change them. Thoughts @soniah & @codedance?

@codedance
Copy link
Contributor

The defaults are there to support the Walk* methods.

I agree that MaxRepetitions should be configurable. I'll change this issue status to "enhancement" and submit a pull request for review.

Regarding OID: You can of course supply your own base OID (and the API encourages this). The default base is there to simply make it easier to not misuse! e.g.

mysnmp.Walk(".1")  // Walk from a very higher level

mysnmp.Walk("1.3.6.1.2.1") // Walk a specific table - e.g.  RFC 1213 system table

mysnmp.Walk("")  // Walk ???? - OK, let's default to the default public space as this should work on every standards compliant device.  Better than throwing a panic :-)

The design is analogous to net-snmp... from the man page:

If no OID argument is present, snmpwalk will search the subtree rooted at SNMPv2-SMI::mib-2

@codedance codedance self-assigned this Jul 17, 2014
codedance added a commit to codedance/gosnmp that referenced this issue Jul 17, 2014
* The walk function uses default values.  It's now possible to
change/override the default values via instance variables on
GoSNMP. Closes issue gosnmp#17 on soniah/gosnmp.
codedance added a commit to codedance/gosnmp that referenced this issue Jul 17, 2014
* The walk function uses default values.  It's now possible to
change/override the default values via instance variables on
GoSNMP. Closes issue gosnmp#17 on soniah/gosnmp.
@codedance
Copy link
Contributor

I've created a pull request of Sonia's master under #18 that should address requirements and close this issue. It's a low risk change but does hit public API surface so a review would be good.

Are we happy with the names?

As an aside:
My only other thinking would be to nest them at a deeper level such as

GoSNMP.Walk.MaxRepetitions
GoSNMP.Walk.NonRepeaters

but I feel the extra code complexity would offset any public surface area gain.

@virtuallynathan
Copy link
Contributor

This was merged, I think we can close this unless @tehnerd has any other comments?

@tehnerd
Copy link
Author

tehnerd commented Jul 23, 2014

sounds good for me. thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants