[U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes

[Marek Vasut <marex@denx.de>]

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