From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Thu, 30 Jan 2020 21:33:57 +0100
Subject: [U-Boot] [PATCH v2 2/3] env: Tidy up some of the env code
In-Reply-To: <0102016eac3ac1a7-8a163dd4-aa1a-4331-a266-e9f461a07db8-000000@eu-west-1.amazonses.com>
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>
Message-ID: <20200130203357.32A8024065D@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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 <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);

       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@denx.de>

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Engineering without management is art."               - Jeff Johnson