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

[Simon Goldschmidt]

Am 30.01.2020 um 21:33 schrieb Wolfgang Denk:
> Dear James,
> 
> In message <0102016eac3ac1a7-8a163dd4-aa1a-4331-a266-e9f461a07db8-000000 at eu-west-1.amazonses.com> you wrote:
>>
>> 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.
> 
> So the 'data' member of 'struct env_entry' should be a "const char
> *", but that does not mean you have to change the interface of
> hsearch_r() ??
> 
>> - 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.
> 
> Well, is is good practice to always initialize the complete sruct.
> Where this is missing, the code should be fixed.
> 
>> 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.
> 
> You miss the point that hsearch_r() actually a standard function,
> see "man 3 hsearch_r":
> 
> HSEARCH(3)                               Linux Programmer's Manual                               HSEARCH(3)
> 
> NAME
>         hcreate, hdestroy, hsearch, hcreate_r, hdestroy_r, hsearch_r - hash table management
> 
> SYNOPSIS
>         #include <search.h>
> 
>         int hcreate(size_t nel);
> 
>         ENTRY *hsearch(ENTRY item, ACTION action);
> 
>         void hdestroy(void);
> 
>         #define _GNU_SOURCE         /* See feature_test_macros(7) */
>         #include <search.h>
> 
>         int hcreate_r(size_t nel, struct hsearch_data *htab);
> 
>         int hsearch_r(ENTRY item, ACTION action, ENTRY **retval,
>                       struct hsearch_data *htab);

Hm, U-Boot's 'hsearch_r' does not conform to this 'standard' since 
December 2012, see these 2 commits from 2012 and 2019:

https://github.com/u-boot/u-boot/commit/c4e0057fa78ebb524b9241ad7245fcd1074ba414

https://github.com/u-boot/u-boot/commit/3f0d6807459bb22431e5bc19e597c1786b3d1ce6

Note I say 'standard' (with quotation marks) because this function seems 
to only be a GNU extension, according to that man page. Nevertheless, it 
does seem to have been adopted by *BSD, even if it hasn't made it to 
opengroups.org (the reference I use when implementing 'standard' calls 
for lwIP).

Obviously, my comments have no real relation to the intention of the 
patch to 'clean up' things. I do think the current situation could be 
improved (e.g. regarding constness), but looking at the nonchalant way 
such a 'standard' function has been change nonstandard, I think this 
should be a change we actively vote for (and the above 2 patches did not 
seem to take this into account).

Regards,
Simon

> 
>         void hdestroy_r(struct hsearch_data *htab);
> 
> 
> I object against changing standard interfaces.
> 
> 
> I also dislike the seocnd part of the patch.  First, this is
> unrelated to the hsearch_r changes, so it should have been a
> separate commit anyway.
> 
> But renaming _do_env_set() into do_interactive_env_set() makes
> absolutely no sense.  It is called in many places from code, which
> hav nothing to do with any interactive mode.  Also, I cannot see
> what you win by splitting two actions that belong together.
> 
> 
> I recommend dropping this patch.
> 
> Naked-by: Wolfgang Denk <wd at denx.de>
> 
> Best regards,
> 
> Wolfgang Denk
>