From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Thu, 30 Jan 2020 21:51:01 +0100 Subject: [U-Boot] [PATCH v2 2/3] env: Tidy up some of the env code In-Reply-To: <20200130203357.32A8024065D@gemini.denx.de> References: <20191121143240.122610-1-james.byrne@origamienergy.com> <0102016e8e613d09-6624acd0-7882-4283-8512-d56fc73489ed-000000@eu-west-1.amazonses.com> <20191127055252.GN22427@linaro.org> <0102016eac3ac1a7-8a163dd4-aa1a-4331-a266-e9f461a07db8-000000@eu-west-1.amazonses.com> <20200130203357.32A8024065D@gemini.denx.de> Message-ID: <11b25950-50a3-c9cd-3c2d-5b2d6583046b@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Am 30.01.2020 um 21:33 schrieb Wolfgang Denk: > Dear James, > > In message <0102016eac3ac1a7-8a163dd4-aa1a-4331-a266-e9f461a07db8-000000@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 > > int hcreate(size_t nel); > > ENTRY *hsearch(ENTRY item, ACTION action); > > void hdestroy(void); > > #define _GNU_SOURCE /* See feature_test_macros(7) */ > #include > > 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 > > Best regards, > > Wolfgang Denk >