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

[Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>]

On Wed, Nov 27, 2019 at 10:39 AM James Byrne
<james.byrne@origamienergy.com> wrote:
>
> 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.

Also, if you add fields without changing the callers to initialize those
new fields, wouldn't you get unpredictive results given that struct env_entry
is often allocated on the stack?

In that case, it might be better to change the signature of hsearch_r() so you
get compiler errors in places that aren't adapted.

Regards,
Simon

>
> Regards,
>
> James