On Tue, 6 Apr 2010 15:46:41 +0100 Dave wrote: DS> Once I'd got over my initial indignation at mere mortals meddling with DS> perfection,
Yes, I know the feeling all to well. ;-) DS> > - use row_merge helper if $i.rowstatus, so handler gets called once per index DS> DS> I can see that this mode of processing *might* be preferable in some DS> circumstances, but I'm unclear why it should be enforced, rather than DS> processing all relevant requests in one go. I don't think there's anything DS> in the code as generated that requires each row to be handled separately. It's primarily for the consistency check in the ACTION phase, which happen after looping through all the columns. If there could be multiple rows being set, you'd have .... Hmphf. Don't you love it when you start to explain why something is needed, with very good logic that you thought long and hard about, and then realize there's another way? :-) I had thought about counting the number of varbinds in RESERVE2, and the using that count in ACTION to determine when the last varbind had been set and thus it was safe to do the consistency checks. But I was worried about other (higher) handlers possibly mucking about with the number of varbinds and screwing up the counts. I remembered that the row_merge helper dealt with this, so it seemed like a an ideal solution. But I think that simply adding another flag to the undo structure and keeping the second loop in ACTION should suffice. DS> It's perhaps overkill to have a -S token to enable or disable this style DS> of working, but at the very least, I'd suggest consolidating all of this DS> initialisation code into one place. so that it can be included/excluded DS> very easily. DS> I.e. move the code that sets up merge_helper to be immediately before DS> the code that registers it. Hmm.. I had it earlier to facilitate the error checking. (Note I also move the cache handler creation earlier for the same reason.) DS> (Oh, and the netsnmp_inject_handler_before() call needs to reference DS> TABLE_TDATA_NAME) Hmm... I just copied what the cache helper did... I'll update both.. DS> > - define a $i_undo struct for use during set processing. DS> DS> Again, I can see that having a separate copy of the full row data DS> structure for undo processing, *might* be preferable to using individual DS> fields. But not necessarily. DS> For example, if there's a large amount of data associated with a row, DS> and very little of this is likely to change, then separate fields might well DS> be more efficient, rather than automatically copying everything. I don't get the argument here... The only thing I did was move where the undo storage was located. In the original scenario, the undo storage is always there, so it can take up significant space for large tables with fields that might contain large values. (Yes, I had to fight the urge to make it an option to use allocated pointers instead of fixed storage.) In either case, the undo data is copied only when the column is set. (Originally I did always copy all the columns, but adding the changed field let me go back to as-needed copies. [See, I'm not just changing stuff willy-nilly! ;-) ]) Though the consistency checks always need to happen. The original code only did them when rowstatus was set. DS> This is probably something where a -S mib2c token could well be DS> sensible, to allow the MIB implementor to choose which approach to use. DS> (And not just for the table_tdata framework - the same choice could DS> well hold true for mib2c.iterator.conf as well. Much of the code is DS> essentially the same, and could usefully be merged). Yes! I almost started a discussion on how we could get all the handlers using the same code, which would allow lots of flexibility for all the handlers. But then I realized that would be much more effort that I had funding for. <sigh> DS> There are a few other bits that I want to have a proper look at before DS> commenting further: DS> DS> > - create temporary row immediately in reserve2 DS> > - generate reserve2 case statements for every col, not just rowstatus, with DS> > commented out simplistic TODO example code DS> > - generate free case statements for every col, not just rowstatus These go hand-in-hand. The original code only generates the rowstatus column case, and that's where it creates the row. But it's entirely possible that other columns might need their own resource allocation, and thus need the row to exist for storing their data. The examples for other case statements simply show the user that the rowstatus column isn't the only one where allocations can/should be done. Of course, if there are per-column allocations, there need to be per-column frees. DS> > - track row fate. i think this might be the same thing as the previous 'valid' flag I initially thought I would do more with fate, particularly with row deletions. In the end I didn't, so likely fate could be renamed back to valid. DS> > - in commit, set notready/notinservice based on earlier consistency checks This is why consistency checks always need to happen in the action phase. A notinservice row that is changed to be inconsistent (which is allowed in this state) should have its rowstatus automatically changed to notready, and vice-versa. DS> [There - that was almost civilised, wasn't it!] Indeed! Amazingly so, considering I'd been mucking about in your pristine code. Well done, old chap! :-) ------------------------------------------------------------------------------ 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
