On Tue, 29 Jun 2004 09:30:45 +0100 Dave wrote:
DS> Robert> A while ago I added a 'flags' field to the mib handler structure.
DS> Robert> One of the flags avaiable is 'AUTO_NEXT', which if set, will call
DS> Robert> the next handler automatically. Seemed this behaviour belonged at
DS> Robert> a higher level. I've updated several handlers to use it,
DS> eliminating Robert> the redundant loop.
DS>
DS> Errr... what loop?
Oops. Replace 'eliminating the redundant loop' with 'eliminating an
unnecessary function call and reducing the stack depth.' (Have you looked at a
back trace from a low level handler? It's horrendous.)
DS> Calling the next handler in the chain is a single statement:
DS>
DS> return netsnmp_call_next_handler(handler, reginfo, reqinfo,
DS> requests);
DS>
DS> That seems just as straightforward as setting
DS>
DS> ret->flags |= MIB_HANDLER_AUTO_NEXT;
Except the flag only has to be set once, during registration. Thus it does
eliminate redundant code (two function calls (call_next just calls call_handler
[which is where we end up if we just return]) times N handlers). I just think
that something that pretty much everyone has to do (and do the same) should be
pushed up the stack, making for one less thing to worry about.
DS> and has the advantage that it occurs within the helper handler routine
DS> itself. I personally find this much clearer than having to look back
DS> at the initialisation code to see what flags may or may not have been set,
DS> and then working out what effect they will have.
Well, that one leaves me speechless.
But just for a little while. Certainly it is clearer. But I know you aren't
proposing that we eliminate flags and instead reproduce code, as a general
principle, for the sake of clarity.
I think that it's that it's a change from how you understood things worked, not
that it's a bad idea. I'd bet if it had worked this way from the beginning, you
wouldn't have thought twice about it. I actually think that this behaviour
should be/have been the default. If I thought I could have gotten away with it,
I would have made the flag "DONT_AUTO_NEXT", so the flag would only need to be
set in the unusual case (I want to affect the decision about how/when to call
lower handlers), instead of the usual case (if there is a next handler, it
should be called).
DS> Maybe I just missed the discussion of this design change, but
DS> I'm not at all convinced that this is an improvement or necessary.
There was no discussion prior to the change, because I never imagined that
there would be an objection to the agent library doing something once instead
of requiring every handler to do it.
Again, it's certainly not necessary, but I maintain that it is an improvement.
Once advantage is maintainability. If some change is made the how the handler
calling chain is made, that change would be localized to the agent library and
any helpers that need to affect the calling change. Without auto-next, every
new-style handler would have to be fixed.
The other is the reduction in stack depth and elimination of two function
calls. I know that modern CPUs really make most optimizations unnecessary, but
the idea of being efficient when possible has been ingrained in my brain. On
the other hand, snmp is also often put into small devices without a lot of
resources, so efficiency can be important to some people in the audience.
DS> Robert> The next level ... would also be to add flags for the various
DS> modes, Robert> so that a handler could indicate it never wanted to be
DS> called for Robert> certain modes.
DS>
DS> That seems more reasonable.
DS> Although you would get exactly the same effect using:
DS>
DS> int some_handler( .... )
DS> {
DS> switch (reqinfo->mode) {
DS> case MODE_THIS:
DS> case MODE_THAT: return SNMP_ERR_NOERROR;
DS>
DS> (which would again be coded "in-line", and hence clearer IMO)
Again, I won't argue the clearer point. But I will play the maintainability
card again. Not only does it require an extra function call (much more
expensive than a test of a flag), it would add one more place that needed to
be updated should the mode stuff ever change (which is already sort of planned;
eg baby-steps).
DS> But I'd still want to raise a note of caution.
DS> One of the strengths of the original handler chain model was the way
DS> that it allowed later handlers to override the effect of earlier ones
DS> (e.g. "fixing" apparent error cases). If the chain isn't invoked at
DS> all for a given mode, then this is no longer possible.
I agree that use of this feature would require caution. But the scenario you
are proposing wouldn't happen when used in conjunction with AUTO_NEXT. Let's
take a simple example. The bulk_to_next helper. There is absolutely no reason
for it to be called for any mode other than get-next. Clear all the other mode
flags, set auto-next, and it is bypassed while still allowing for lower levels
to be called for all modes.
DS> That's not necessarily a problem, of course - if something is declared
DS> as read-only, then it's perfectly reasonable to block all SET requests.
DS> But it's something that should be used with restraint.
I disagree. It should be used, in conjunction with auto-next, everywhere
possible. The shallower the stack dept, and the fewer unnecessary function
calls we make, the better.
DS> But this feels somewhat less confusing than the AUTO_NEXT behaviour,
DS> which I really don't think is necessary. But I'm probably in a
DS> minority of one yet again.
Well, I don't know if any of the other core developers are going to speak up.
When I checked in the code, Wes said "good idea", so unless he has been swayed
by your argument for clarity, it stands at 2-1.
That being said, the cache handler is your code, and it will certainly work
just find if the flag was removed and the explicit call added back. And in
keeping with Wes' rule that we don't try and force any particular coding style
(within reason) on anyone, just say the word and I'll revert the cache handler
to the original style.
However, unless there are objections to the whole concept, most of the other
handlers are in the queue for the chopping block to be updated to use
auto-next (bulk to next being on the top of the list).
--
Robert Story; NET-SNMP Junkie <http://www.net-snmp.org/>
<irc://irc.freenode.net/#net-snmp>
Archive: <http://sourceforge.net/mailarchive/forum.php?forum=net-snmp-coders>
You are lost in a twisty maze of little standards, all different.
-------------------------------------------------------
This SF.Net email sponsored by Black Hat Briefings & Training.
Attend Black Hat Briefings & Training, Las Vegas July 24-29 -
digital self defense, top technical experts, no vendor pitches,
unmatched networking opportunities. Visit www.blackhat.com
_______________________________________________
Net-snmp-coders mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
