On Fri, 16 Sep 2005 15:07:55 +0100 Dave wrote:
DS> Some time ago, there was a brief discussion about what
DS> operations were necessary for implementing and using
DS> a table helper (regardless of the internal details
DS> of exactly how that particular helper worked).
DS>
DS> I'd be interested in hearing what people think about
DS> this general framework.
Ok, but remember, you asked for it! :-)
> typedef struct netsnmp_tdata_row_s {
> netsnmp_index oid_index; /* table_container index format */
> netsnmp_variable_list *indexes; /* stored permanently if
> store_indexes = 1 */
> void *data; /* the data to store */
>
> struct netsnmp_tdata_row_s *next, *prev; /* if used in a list
> */ } netsnmp_tdata_row;
Hmmm... I wish we had more time before 5.3. I'd like to see the index/*data
stuff moved inside the container stuff.
I don't like having two index methods in the structure. It's a waste of space.
I'd rather see a union, and let the user determine which they want to use.
(You did know you could use containers without netsnmp_index, right?) The
internal storage shouldn't really be visible to the user, anyways. The API
should hide the details (ie netsnmp_tdata_generate_index_oid() should *not* be
a public API. Instead, there should be netsnmp_tdata_index_set(netsnmp_index*)
netsnmp_tdata_index_set_from_vb(netsnmp_variable_list*).)
I also think that next/prev pointers are a complete waste, and I object pretty
strongly to them being included by default. It's trivial for someone to add
them if needed.
I also think 'tdata_row' is redundant. What other kind of row is there? This
is a nice generic API, how about simply trow? A table row API and a table data
API?
> typedef struct netsnmp_tdata_s {
> netsnmp_variable_list *indexes_template; /* containing only
> types */ char *name; /* if !NULL, it's registered globally */
> int flags; /* not currently used */
> int store_indexes;
> netsnmp_container *container;
> } netsnmp_tdata;
I really hate to see an int used for a boolean. (It still bugs me that we don't
use the C construct for named bits.) It doesn't even make any sense, because
you've got 32 bits of an unused 'flag' member!
I was surprised that this didn't include something to deal with saving default
values for columns, to be used during row creation.
> int netsnmp_tdata_add_row();
> void *netsnmp_tdata_delete_row();
> void *netsnmp_tdata_remove_and_delete_row();
> netsnmp_tdata_row *netsnmp_tdata_remove_row();
> netsnmp_tdata_row *netsnmp_tdata_create_row(void);
> netsnmp_tdata_row *netsnmp_tdata_clone_row(netsnmp_tdata_row *row);
I though we agreed on object before function in function names? eg
netsnmp_tdata_row_(add|delete|remove|...).
Why does remove_and_delete return void, but remove_row return a tdata_row?
> netsnmp_tdata_replace_row();
This makes me nervous... Why wouldn't they just copy over the data
pointer/flags? Look at the code, it seems a silly wrapper, and doesn't even
check if it's really a replacement (eg indexes match). I suggest getting rid
of this function.
> netsnmp_tdata *netsnmp_tdata_create(const char *name);
create what? (note, if tdata_row is renamed to trow, this is no nonger
ambiguous and objection is withdrawn).
> #define netsnmp_tdata_add_index(thetable, type)
> snmp_varlist_add_variable(&thetable->indexes_template, NULL, 0, type, NULL,
> 0) #define netsnmp_tdata_row_add_index(row, type, value, value_len)
> snmp_varlist_add_variable(&row->indexes, NULL, 0, type, (const u_char *)
> value, value_len)
I dislike macro function mapping in general, and even less so when they
de-reference pointers without null checks. I suggest one of the following
(in preference order)
1) making these inline functions (handling case where inline isn't available)
2) making these real functions
3) expand macros to include error checking
> netsnmp_tdata_row* netsnmp_tdata_get_first_row(netsnmp_tdata *table);
> netsnmp_tdata_row* netsnmp_tdata_get_next_row( netsnmp_tdata *table,
> netsnmp_tdata_row *row);
Suggest a wrapper using the new container iterator stuff too, which would be
much more efficient for someone traversing the whole table.
Well, that's it off the top of my head.
--
Robert Story; NET-SNMP Junkie
Support: <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 is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Net-snmp-coders mailing list
Net-snmp-coders@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
