Dear Gerlando Falauto,
On 03/30/2012 03:55 PM, Marek Vasut wrote:
Dear Gerlando Falauto,
On 03/30/2012 03:08 PM, Marek Vasut wrote:
Dear Gerlando Falauto,
On 03/29/2012 10:19 PM, Marek Vasut wrote:
[...]
> + return 0; > + } > +#endif > + return 0; > +} > +
[...]
> --- a/include/search.h > +++ b/include/search.h > @@ -47,6 +47,13 @@ typedef struct entry { > > struct _ENTRY; > > /* > > + * Callback function to be called for checking whether the given > change may + * be applied or not. Must return 0 for approval, 1 for > denial. + */ > +typedef int (*apply_cb)(const char *name, const char *oldval, > + const char *newval, int flag);
Is the typedef really necessary ?
[From your other email]
I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?
Why ask, if you already know the answer? :-)
I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data).
What if the function pointer was stored within the hastable itself? Sort of a virtual method. This way we get rid of the typedef and the function pointer as a parameter altogether. The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think?
Don't we always use only one (this callback) function?
Yes, but only because env is the only hashtable present. Is that a yes or a no, then?
Do we expect any more hashtables in the near future ?
I don't think so. Anyway I would rather avoid calling functions belonging to the environment domain from the hastable domain directly. For that matter, we have a single "struct hsearch_data" instance in the whole project, but we keep passing it around as an argument to the hashtable functions.
Ah, it just came to me.
On the other hand, I have this strange feeling lib/hashtable.c is so modified for uboot it can't be used (in a generic way) for any other storage than env ... or am I wrong?
WD, can you comment please?
Best, Gerlando
Best regards, Marek Vasut