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

[Marek Vasut]

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:
> >>> Dear Gerlando Falauto,
> >>> 
> >>> WD prodded me too long to review this patchset ;-D
> >> 
> >> Well, better late than never! ;-)
> >> 
> >> [...]
> >> 
> >>>> +#if defined(CONFIG_CMD_NET)
> >>>> +	else if (strcmp(name, "bootfile") == 0) {
> >>>> +		copy_filename(BootFile, newval, sizeof(BootFile));
> >>> 
> >>> Can you remove the camel-case here please?
> >> 
> >> That's code I just moved around... Will do, sir.
> > 
> > Don't call me that way, makes me feel old :D
> 
> Was more of a way to remark authority than age. :-)

;-)

> >>>> +		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 ?

> >> [...]
> >> 
> >>>>    /* Flags for himport_r() */
> >>>>    #define	H_NOCLEAR	1	/* do not clear hash table 
before
> >>> 
> >>> importing */
> >>> 
> >>>> +#define H_FORCE		2	/* overwrite read-only/write-once
> >>> 
> >>> variables */
> >>> 
> >>> Make this 1<<   x please.
> >> 
> >> OK.
> >> 
> >>>>    #endif /* search.h */
> >>>> 
> >>>> diff --git a/lib/hashtable.c b/lib/hashtable.c
> >>>> index abd61c8..75b9b07 100644
> >>>> --- a/lib/hashtable.c
> >>>> +++ b/lib/hashtable.c
> >>>> @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab,
> >>>> const char sep, * himport()
> >>>> 
> >>>>     */
> >>>> 
> >>>> +/* Check whether variable name is amongst vars[] */
> >>>> +static int process_var(const char *name, int nvars, char * const
> >>>> vars[])
> >>> 
> >>> You mean check_var()?
> >> 
> >> I mean is_var_in_set_or_is_set_empty().
> > 
> > Nice name :)
> > 
> >> Sorry, I'm very, very bad at picking function names.
> >> Any suggestion?
> > 
> > The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't
> > be sorry, you're doing very good job!
> 
> I like is_var_in_set().

So be it then ;-)

Thanks!

> Thanks,
> Gerlando