[U-Boot] [PATCH v2 2/3] env: Tidy up some of the env code

[James Byrne]

On 27/11/2019 05:52, AKASHI Takahiro wrote:
> On Thu, Nov 21, 2019 at 02:32:47PM +0000, James Byrne wrote:
>> This commit tidies up a few things in the env code to make it safer and
>> easier to extend:
>>
>> - The hsearch_r() function took a 'struct env_entry' as its first
>> parameter, but only used the 'key' and 'data' fields. Some callers would
>> set the other fields, others would leave them undefined. Another
>> disadvantage was that the struct 'data' member is a 'char *', but this
>> function does not modify it, so it should be 'const char *'. To resolve
>> these issues the function now takes explcit 'key' and 'data' parameters
>> that are 'const char *', and all the callers have been modified.
> 
> I don't have a strong opinion here, but we'd rather maintain the current
> interface. Yes, currently no users use any fields other than key/data,
> but in the future, this function may be extended to accept additional
> *search* parameters in a key, say flag?. At that time, we won't have to
> change the interface again.

As I said in my commit log comment, there are two key arguments against 
this:

- The fact that the 'data' member of 'struct env_entry' is a 'char *' is 
really inconvenient because this is a read-only function where most of 
the callers should be using 'const char *' pointers, and having to cast 
away the constness isn't good practice and makes the calling code less 
readable.

- As you can see from the calling code I've had to tidy up, the callers 
were very inconsistent about whether they bothered to initialise any 
fields other than 'key' and 'value', so if you ever wanted to extend the 
interface to check other parameters you'd have to go around and fix them 
all up anyway to avoid unpredictable behaviour.

Given that only 'key' and 'value' are used at the moment I think my 
change is preferable because it makes it explicit what is being used and 
avoids any nasty surprises you might get if you changed hsearch_r() 
without changing all the callers. If you anticipate wanting to match on 
other fields, it might be better to define an alternative query 
structure using 'const char *' pointers for key and value, then extend 
that, but I would argue that that's something you could do at the point 
you find it is needed rather than now.

Regards,

James