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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
