On 6 April 2010 11:59, Robert Story <[email protected]> wrote:
> I'm working on some new tables and decided to give the table_data conf file a
> whirl....... I've made quite a few changes.
> Some minor, some major....
> I haven't checked it in yet because I wanted to make sure I hadn't strayed too
> far from the original author's intent.
OK - I've had a quick look at the changes (mostly looking at the effects
on the generated output, since that's probably the most important aspect).
Once I'd got over my initial indignation at mere mortals meddling with
perfection, I think there are really only two main areas of concern:
> - use row_merge helper if $i.rowstatus, so handler gets called once per index
I can see that this mode of processing *might* be preferable in some
circumstances, but I'm unclear why it should be enforced, rather than
processing all relevant requests in one go. I don't think there's anything
in the code as generated that requires each row to be handled separately.
It's perhaps overkill to have a -S token to enable or disable this style
of working, but at the very least, I'd suggest consolidating all of this
initialisation code into one place. so that it can be included/excluded
very easily.
I.e. move the code that sets up merge_helper to be immediately before
the code that registers it.
(Oh, and the netsnmp_inject_handler_before() call needs to reference
TABLE_TDATA_NAME)
> - define a $i_undo struct for use during set processing.
Again, I can see that having a separate copy of the full row data
structure for undo processing, *might* be preferable to using individual
fields. But not necessarily.
For example, if there's a large amount of data associated with a row,
and very little of this is likely to change, then separate fields might well
be more efficient, rather than automatically copying everything.
This is probably something where a -S mib2c token could well be
sensible, to allow the MIB implementor to choose which approach to use.
(And not just for the table_tdata framework - the same choice could
well hold true for mib2c.iterator.conf as well. Much of the code is
essentially the same, and could usefully be merged).
There are a few other bits that I want to have a proper look at before
commenting further:
> - create temporary row immediately in reserve2
> - generate reserve2 case statements for every col, not just rowstatus, with
> commented out simplistic TODO example code
> - generate free case statements for every col, not just rowstatus
> - track row fate. i think this might be the same thing as the previous
> 'valid' flag
> - in commit, set notready/notinservice based on earlier consistency checks
But the rest of the changes look sensible enough, so I've no concerns there.
The two mentioned above are the only real issues in my mind.
Dave
[There - that was almost civilised, wasn't it!]
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Net-snmp-coders mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
