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

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

>
>>
>> [...]
>>
>>>>    /* 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().

Thanks,
Gerlando