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

Marek Vasut
2012-03-30T14:28:01Z

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