bsnmpd(1): optimize interface description processing

[eugen_grosbein.net]

Summary

• teach bsnmpd to allocate memory dynamically for interface descriptions

to decrease memory usage for common case and not to break if long description occurs;

• skip calling strlen() for each description and each SNMP request for MIB-II/ifXTable's ifAlias.

Test Plan

1. Do:

ifconfig ng0 descr "70-chars long description"
ifconfig ng1 descr "70-chars long description"

and observe bsnmpd spamming logs with messages:

SIOCGIFDESCR (ng0): too long (70)
SIOCGIFDESCR (ng1): too long (70)

And empty ifAlias'es in the ifXTable instead of descriptions.

2. Apply the patch, rebuild and restart bsnmpd and see no errors and correctly filles ifAlias values.

[avg]

Some comments inline, but in general looks good to me.
Thanks!

contrib/bsnmp/snmp_mibII/mibII.c

448 ↗

(On Diff #45878)

I would order this static variable before other variables and then group the new non-static variables with the existing variables.

449 ↗

(On Diff #45878)

Is nitmes available to userland? If yes, then it would be better to use it.

539 ↗

(On Diff #45878)

I guess this should have been kmib[0] > 0 ?

760 ↗

(On Diff #45878)

Missing space after if.
Also, it would be more canonical to explicitly compare with NULL.

[eugen_grosbein.net]

contrib/bsnmp/snmp_mibII/mibII.c

448 ↗

(On Diff #45878)

Ok

449 ↗

(On Diff #45878)

Yes, it is available.

539 ↗

(On Diff #45878)

The case kmib[0] < 0 is processed above, so if (kmib[0]) is, indeed, if (kmib[0] > 0) but shorter as later variant pushes length of this line to its limits. Anyway, I change this.

760 ↗

(On Diff #45878)

Ok

[eugen_grosbein.net]

Style changes.

[bz]

Hmm RFC 2863 says:

ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

So where is our real problem?

[bz]

I guess I want the real problem to be identified and understood (or a good description for this change and why) rather than patching something that simply masks a different problem.

[eugen_grosbein.net]

In D16459#349396, @bz wrote:

Hmm RFC 2863 says:

ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

Nice catch. Does it mean that complying SNMP application must support at most 64 (or 65) octects lenght ifAlias or it must support at least so? Note that Cisco supports "at least" or more: http://cfn.cloudapps.cisco.com/ITDIT/CFN/Dispatch?act=featdesc&task=display&featureId=742

So where is our real problem?

"Test plan" section describes our problems with bsnmpd:

• bsnmpd spams logs indefinitly if a system has interface with description over 65 characters;
• returned ifAlias is inaccurately empty for such an interface;
• every system interface increases size of bsnmpd data set for next char[65] array no matter if it is needed or not, when an interface has no description at all;
• every SNMP request for ifXTable/ifAlias OID results in ineffective strlen() call for mentioned array.

Proposed patch fixed all these problems.

[bz]

In D16459#349406, @eugen_grosbein.net wrote:

In D16459#349396, @bz wrote:

Hmm RFC 2863 says:

ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

Nice catch. Does it mean that complying SNMP application must support at most 64 (or 65) octects lenght ifAlias or it must support at least so? Note that Cisco supports "at least" or more: http://cfn.cloudapps.cisco.com/ITDIT/CFN/Dispatch?act=featdesc&task=display&featureId=742

So where is our real problem?

"Test plan" section describes our problems with bsnmpd:

• bsnmpd spams logs indefinitly if a system has interface with description over 65 characters;

Yes but that might not be bsnmpd's fault (well it might with the "spamming") but should the system allow them to be set longer? Should they be silently truncated when exporting (or being read by bsnmpd)? Just masking that problem away is what I mean doesn't exist. What I am saying is that if this patch is considered to be what "we want" then we should document the possible violation of the RFC (hopefully only if the user changes a sysctl elsewhere, or if not under which circumstances) somewhere (ideally in a man page). But someone needs to make a decision on why the base system behaves the way it does, whether that is the right thing, or if not where we'll "break" the standard.

[eugen_grosbein.net]

In D16459#349416, @bz wrote:

Yes but that might not be bsnmpd's fault (well it might with the "spamming")

That is definitely bsnmp's problem.

but should the system allow them to be set longer?

The kernel has no SNMP support. But it has interface descriptions limited by sysctl net.ifdescr_maxlen == 1024 by default.
bsnmpd obtains descriptions using sysctl(3) interface and short 65-octet buffer and fails.

Should they be silently truncated when exporting (or being read by bsnmpd)? Just masking that problem away is what I mean doesn't exist.

Why do you calling it "masking"? Have you actually read the patch? It solves the problem, not masks it.

What I am saying is that if this patch is considered to be what "we want" then we should document the possible violation of the RFC (hopefully only if the user changes a sysctl elsewhere, or if not under which circumstances) somewhere (ideally in a man page).

Why would be that "violation" of the RFC? I could not find a prohibition to support longer ifAlias anywhere in RFC, and Cisco supports longer values too. And patched bsnmpd won't emit longer ifAlias'es unless system administrator or software configures an interface with long description.

[avg]

Well, the standard says (0 ..64) and that means from zero to 64.
Also, my impression is that, according to the RFC, ifAlias is something that should be settable over SNMP and it should be persistent.

I found this interesting link on the topic and I must admit that I know very little about it:
http://irobert.info/display/IRINFO/Understanding+ifDescr%2C+ifName+and+ifAlias

[harti]

In D16459#349483, @avg wrote:

Well, the standard says (0 ..64) and that means from zero to 64.
Also, my impression is that, according to the RFC, ifAlias is something that should be settable over SNMP and it should be persistent.

I found this interesting link on the topic and I must admit that I know very little about it:
http://irobert.info/display/IRINFO/Understanding+ifDescr%2C+ifName+and+ifAlias

I wish I had this reference when writing the code in the first place :-).

I looked up RFC2578. Section 9 (Refined Syntax) says for OCTET STRING:

(3) the size in octets of the value may be refined by raising the

lower-bounds, by reducing the upper-bounds, and/or by reducing the
alternative size choices.

Below that:

No other types of refinements can be specified in the SYNTAX clause.

I think the point here is to allow dumb clients with static buffers to handle all legal cases.

I would recommend to just silently clip the description at the maximum size.

[eugen_grosbein.net]

In D16459#349483, @avg wrote:

Well, the standard says (0 ..64) and that means from zero to 64.
Also, my impression is that, according to the RFC, ifAlias is something that should be settable over SNMP and it should be persistent.

bsnmpd does not support SNMP SET for ifAlias currently

[eugen_grosbein.net]

The only change comparing with previous revision is added truncation of interface description obtained with sysctl(3) if it appears longer than 64 octets.

[eugen_grosbein.net]

I'm going to commit latest revision soon unless an objection is raised.

[avg]

In D16459#350746, @eugen_grosbein.net wrote:

I'm going to commit latest revision soon unless an objection is raised.

You would still need a mentor approval.