On Sat, 2005-10-01 at 11:20 -0400, Robert Story wrote:
> DS> I'd be interested in hearing what people think about
> DS> this general framework.
>
> Ok, but remember, you asked for it! :-)
Thanks for those comments, Robert.
There's definitely a lot of useful stuff there.
I get the impression that most of what you've said is
concerned with the specifics of this particular helper.
Did you have any thoughts about this as an example of
a more general API framework? Any operations that
felt to be missing?
Turning to the specifics:
> > typedef struct netsnmp_tdata_row_s {
[snip]
> > } 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.
Remember that this data structure is basically part of the internals
of the tdata helper. It's not intended to be accessed directly
by the programmer - it's effectively an opaque box. So we will
be able to adjust the precise details later - just as long as
the visible APIs stay consistent.
> I also think 'tdata_row' is redundant. What other kind of row is there?
There are "table-independent" row structures (tdata_row), wrapped
round "table-specific" row structures (cast to a void* internally).
I'd have preferred to use the more general name "netsnmp_table_row",
but that was already defined (as part of the original table_data
helper), and didn't seem to be compatible with the table_container
mechanism.
> (You did know you could use containers without netsnmp_index, right?)
Ummm... no.
I'd got the impression that the 'table_container' helper *did*
require netsnmp_index. See the comments immediately following the
three key types in table_container.c. Is this wrong?
If so, then we could revert to using 'netsnmp_table_row' again
(but see later).
> This is a nice generic API, how about simply trow? A table row API
> and a table data API?
I'm not quite sure what you mean by this. Is this the same
table-independent/table-specific distinction mentioned above,
or something different?
> 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.
No - this is an internal structure, and the user shouldn't be fiddling
with it (or even looking at it). Think in terms of the distinction
between snmp_session and snmp_sess_session.
> The internal storage shouldn't really be visible to the user, anyways. The API
> should hide the details
Exactly.
> (ie netsnmp_tdata_generate_index_oid() should *not* be a public API.
Good point - there's no reason for making this visible at all.
I'll hide it.
> I also think that next/prev pointers are a complete waste, and I object
> pretty strongly to them being included by default.
Agreed. I'll remove them.
Both of these (as with some of the rest of your comments)
are hang-overs from the original table_data implementation.
> It's trivial for someone to add them if needed.
Not to the (internal) netsnmp_tdata_row structure, they couldn't!
But the table-specific per-row data structure is fair game.
> > typedef struct netsnmp_tdata_s {
:
> > int flags; /* not currently used */
> > int store_indexes;
> > netsnmp_container *container;
> > } netsnmp_tdata;
>
> I really hate to see an int used for a boolean.
You mean 'store_indexes', I presume?
You're quite correct - another relic of the table_data ancestry.
I'm uncertain whether the user should be concerned with this
at all, but using a bit of the flags field would be more sensible.
> I was surprised that this didn't include something to deal with
> saving default values for columns, to be used during row creation.
I'm not at all sure what you mean by that.
Rather than responding to a (probably erroneous) guess, perhaps you
could expand a bit?
> > #define netsnmp_tdata_add_index(thetable, type)...
> > #define netsnmp_tdata_row_add_index(row, type, value, value_len)...
>
> I dislike macro function mapping in general, and even less so when
> they de-reference pointers without null checks.
Fair comment.
I'll pass the buck onto the table_data code ancestry again :-)
> I suggest
> 1) making these inline functions
I'd be happy with that.
<adds to the ToDo list>
> > netsnmp_tdata_replace_row();
>
> This makes me nervous... Why wouldn't they just copy over the data
> pointer/flags?
'Cos that's meddling with the internals of a (private, opaque)
data structure!
> Look at the code, it seems a silly wrapper.... I suggest
> getting rid of this function.
I'm not at all sure of the purpose of this routine - it's another
carry-over from the table_data code. My *guess* is that it's
probably somehow related to the handling of SET requests (perhaps
using a safety copy of the whole row, rather than individual
"old_value" column fields).
I'll drop it unless/until we come up with a clearer idea of
the intended use.
> > 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.
But these routines already use the CONTAINER_{FIRST,NEXT} macros.
Won't that pick up the appropriate container iterator mechanism?
I can't help feeling that I've missed the point here.
> > void *netsnmp_tdata_remove_and_delete_row();
> > netsnmp_tdata_row *netsnmp_tdata_remove_row();
> Why does remove_and_delete return void, but remove_row return a tdata_row?
It's a matter of memory that has been released, or is still valid.
'netsnmp_tdata_remove_row()' removes the (table-independent) row
structure from the table, and returns it. All memory is valid.
'netsnmp_tdata_remove_and_delete_row()' removes the same row
structure from the table, releases the table-independent portion,
and returns the table-specific entry (as a void*, not void).
The tdata_row pointer is no longer valid memory, but the
table-specific pointer is (since it may need special handling).
Anyway, enough of this general agreement - it's boring!
> > 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|...).
The trouble with those names is that they are ambigous.
Does 'netsnmp_tdata_row_add' mean
"add a row to a tdata table"
or
"add a tdata_row (to something - what?)"
RPN is fine for scientific calculators, but us mere humans are
usually happier with "operand operator operand" syntax :-)
So I'd agree with using a common prefix (netsnmp_tdata)
for this API, but I'm reluctant to adopt too deep a stack
of operands.
> > netsnmp_tdata *netsnmp_tdata_create(const char *name);
>
> create what?
Create a "tdata" table.
Which rather supports the point above.
RPN-style naming is confusing enough with a single operand.
It's even worse with multiple operands.
> (note, if tdata_row is renamed to trow, this is no nonger
> ambiguous and objection is withdrawn).
Eh?
I don't follow that at all.
This API doesn't make any mention of rows whatsoever???
Thanks for the comments, anyway.
Dave
-------------------------------------------------------
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
