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

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.

- Break up _do_env_set() so that it only does the argument handling,
rename it to do_interactive_env_set() and use 'const char *' pointers
for argv. The actual variable modification has been split out to two
separate functions, do_env_remove() and do_env_update(), which can also
be called from the programmatic version env_set(), meaning it no longer
has to create fake command line parameters. The do_interactive_env_set()
function is not required in SPL builds.

- Fix some warnings identified by checkpatch.pl

Signed-off-by: James Byrne <james.byrne@origamienergy.com>

---

Changes in v2:
- Fix checkpatch.pl so that this patchset can pass without warnings.
- Tidy up the underlying code before adding env_force_set()
- Rename new function from env_force() to env_force_set()

 api/api.c             |   5 +-
 cmd/nvedit.c          | 111 +++++++++++++++++++++++-------------------
 drivers/tee/sandbox.c |  17 +++----
 env/callback.c        |   7 +--
 env/flags.c           |   7 +--
 include/search.h      |   2 +-
 lib/hashtable.c       |  83 ++++++++++++++++---------------
 test/env/hashtable.c  |  23 ++-------
 8 files changed, 119 insertions(+), 136 deletions(-)

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.

-Takahiro Akashi


> - Break up _do_env_set() so that it only does the argument handling,
> rename it to do_interactive_env_set() and use 'const char *' pointers
> for argv. The actual variable modification has been split out to two
> separate functions, do_env_remove() and do_env_update(), which can also
> be called from the programmatic version env_set(), meaning it no longer
> has to create fake command line parameters. The do_interactive_env_set()
> function is not required in SPL builds.
> 
> - Fix some warnings identified by checkpatch.pl
> 
> Signed-off-by: James Byrne <james.byrne@origamienergy.com>
> 
> ---
> 
> Changes in v2:
> - Fix checkpatch.pl so that this patchset can pass without warnings.
> - Tidy up the underlying code before adding env_force_set()
> - Rename new function from env_force() to env_force_set()
> 
>  api/api.c             |   5 +-
>  cmd/nvedit.c          | 111 +++++++++++++++++++++++-------------------
>  drivers/tee/sandbox.c |  17 +++----
>  env/callback.c        |   7 +--
>  env/flags.c           |   7 +--
>  include/search.h      |   2 +-
>  lib/hashtable.c       |  83 ++++++++++++++++---------------
>  test/env/hashtable.c  |  23 ++-------
>  8 files changed, 119 insertions(+), 136 deletions(-)
> 
> diff --git a/api/api.c b/api/api.c
> index 71fa03804e..b950d8cbb7 100644
> --- a/api/api.c
> +++ b/api/api.c
> @@ -514,7 +514,7 @@ static int API_env_enum(va_list ap)
>  {
>  	int i, buflen;
>  	char *last, **next, *s;
> -	struct env_entry *match, search;
> +	struct env_entry *match;
>  	static char *var;
>  
>  	last = (char *)va_arg(ap, unsigned long);
> @@ -530,8 +530,7 @@ static int API_env_enum(va_list ap)
>  		s = strchr(var, '=');
>  		if (s != NULL)
>  			*s = 0;
> -		search.key = var;
> -		i = hsearch_r(search, ENV_FIND, &match, &env_htab, 0);
> +		i = hsearch_r(var, NULL, ENV_FIND, &match, &env_htab, 0);
>  		if (i == 0) {
>  			i = API_EINVAL;
>  			goto done;
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 99a3bc57b1..b30669a45e 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -94,11 +94,9 @@ static int env_print(char *name, int flag)
>  	ssize_t len;
>  
>  	if (name) {		/* print a single name */
> -		struct env_entry e, *ep;
> +		struct env_entry *ep;
>  
> -		e.key = name;
> -		e.data = NULL;
> -		hsearch_r(e, ENV_FIND, &ep, &env_htab, flag);
> +		hsearch_r(name, NULL, ENV_FIND, &ep, &env_htab, flag);
>  		if (ep == NULL)
>  			return 0;
>  		len = printf("%s=%s\n", ep->key, ep->data);
> @@ -217,15 +215,55 @@ DONE:
>  #endif
>  #endif /* CONFIG_SPL_BUILD */
>  
> +static int do_env_remove(const char *name, int env_flag)
> +{
> +	int rc;
> +
> +	env_id++;
> +
> +	rc = hdelete_r(name, &env_htab, env_flag);
> +	return !rc;
> +}
> +
> +static int do_env_update(const char *name, const char *value, int env_flag)
> +{
> +	struct env_entry *ep;
> +
> +	env_id++;
> +
> +	hsearch_r(name, value, ENV_ENTER, &ep, &env_htab, env_flag);
> +	if (!ep) {
> +		printf("## Error inserting \"%s\" variable, errno=%d\n",
> +		       name, errno);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +int env_set(const char *varname, const char *varvalue)
> +{
> +	/* before import into hashtable */
> +	if (!(gd->flags & GD_FLG_ENV_READY))
> +		return 1;
> +
> +	if (!varvalue || varvalue[0] == '\0')
> +		return do_env_remove(varname, H_PROGRAMMATIC);
> +
> +	return do_env_update(varname, varvalue, H_PROGRAMMATIC);
> +}
> +
> +#ifndef CONFIG_SPL_BUILD
>  /*
>   * Set a new environment variable,
>   * or replace or delete an existing one.
>   */
> -static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
> +static int do_interactive_env_set(int flag, int argc, const char * const argv[])
>  {
> -	int   i, len;
> -	char  *name, *value, *s;
> -	struct env_entry e, *ep;
> +	int env_flag = H_INTERACTIVE;
> +	int i, len, rc;
> +	const char *name;
> +	char *value, *s;
>  
>  	debug("Initial value for argc=%d\n", argc);
>  
> @@ -235,7 +273,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>  #endif
>  
>  	while (argc > 1 && **(argv + 1) == '-') {
> -		char *arg = *++argv;
> +		const char *arg = *++argv;
>  
>  		--argc;
>  		while (*++arg) {
> @@ -257,12 +295,9 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>  		return 1;
>  	}
>  
> -	env_id++;
> -
>  	/* Delete only ? */
>  	if (argc < 3 || argv[2] == NULL) {
> -		int rc = hdelete_r(name, &env_htab, env_flag);
> -		return !rc;
> +		return do_env_remove(name, env_flag);
>  	}
>  
>  	/*
> @@ -277,7 +312,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>  		return 1;
>  	}
>  	for (i = 2, s = value; i < argc; ++i) {
> -		char *v = argv[i];
> +		const char *v = argv[i];
>  
>  		while ((*s++ = *v++) != '\0')
>  			;
> @@ -286,32 +321,12 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>  	if (s != value)
>  		*--s = '\0';
>  
> -	e.key	= name;
> -	e.data	= value;
> -	hsearch_r(e, ENV_ENTER, &ep, &env_htab, env_flag);
> +	rc = do_env_update(name, value, env_flag);
>  	free(value);
> -	if (!ep) {
> -		printf("## Error inserting \"%s\" variable, errno=%d\n",
> -			name, errno);
> -		return 1;
> -	}
>  
> -	return 0;
> -}
> -
> -int env_set(const char *varname, const char *varvalue)
> -{
> -	const char * const argv[4] = { "setenv", varname, varvalue, NULL };
> -
> -	/* before import into hashtable */
> -	if (!(gd->flags & GD_FLG_ENV_READY))
> -		return 1;
> -
> -	if (varvalue == NULL || varvalue[0] == '\0')
> -		return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC);
> -	else
> -		return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC);
> +	return rc;
>  }
> +#endif
>  
>  /**
>   * Set an environment variable to an integer value
> @@ -382,7 +397,7 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	if (argc < 2)
>  		return CMD_RET_USAGE;
>  
> -	return _do_env_set(flag, argc, argv, H_INTERACTIVE);
> +	return do_interactive_env_set(flag, argc, (const char * const *)argv);
>  }
>  
>  /*
> @@ -393,7 +408,7 @@ int do_env_ask(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>  	char message[CONFIG_SYS_CBSIZE];
>  	int i, len, pos, size;
> -	char *local_args[4];
> +	const char *local_args[4];
>  	char *endptr;
>  
>  	local_args[0] = argv[0];
> @@ -460,7 +475,7 @@ int do_env_ask(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	}
>  
>  	/* Continue calling setenv code */
> -	return _do_env_set(flag, len, local_args, H_INTERACTIVE);
> +	return do_interactive_env_set(flag, len, local_args);
>  }
>  #endif
>  
> @@ -643,12 +658,12 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
>  	if (buffer[0] == '\0') {
>  		const char * const _argv[3] = { "setenv", argv[1], NULL };
>  
> -		return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE);
> +		return do_interactive_env_set(0, 2, _argv);
>  	} else {
>  		const char * const _argv[4] = { "setenv", argv[1], buffer,
>  			NULL };
>  
> -		return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE);
> +		return do_interactive_env_set(0, 3, _argv);
>  	}
>  }
>  #endif /* CONFIG_CMD_EDITENV */
> @@ -662,13 +677,11 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
>  char *env_get(const char *name)
>  {
>  	if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */
> -		struct env_entry e, *ep;
> +		struct env_entry *ep;
>  
>  		WATCHDOG_RESET();
>  
> -		e.key	= name;
> -		e.data	= NULL;
> -		hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
> +		hsearch_r(name, NULL, ENV_FIND, &ep, &env_htab, 0);
>  
>  		return ep ? ep->data : NULL;
>  	}
> @@ -1262,14 +1275,12 @@ static int do_env_info(cmd_tbl_t *cmdtp, int flag,
>  static int do_env_exists(cmd_tbl_t *cmdtp, int flag, int argc,
>  		       char * const argv[])
>  {
> -	struct env_entry e, *ep;
> +	struct env_entry *ep;
>  
>  	if (argc < 2)
>  		return CMD_RET_USAGE;
>  
> -	e.key = argv[1];
> -	e.data = NULL;
> -	hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
> +	hsearch_r(argv[1], NULL, ENV_FIND, &ep, &env_htab, 0);
>  
>  	return (ep == NULL) ? 1 : 0;
>  }
> diff --git a/drivers/tee/sandbox.c b/drivers/tee/sandbox.c
> index 4b91e7db1b..92467ba89b 100644
> --- a/drivers/tee/sandbox.c
> +++ b/drivers/tee/sandbox.c
> @@ -79,7 +79,7 @@ static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params,
>  			      struct tee_param *params)
>  {
>  	struct sandbox_tee_state *state = dev_get_priv(dev);
> -	struct env_entry e, *ep;
> +	struct env_entry *ep;
>  	char *name;
>  	u32 res;
>  	uint slot;
> @@ -172,9 +172,7 @@ static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params,
>  		value = params[1].u.memref.shm->addr;
>  		value_sz = params[1].u.memref.size;
>  
> -		e.key = name;
> -		e.data = NULL;
> -		hsearch_r(e, ENV_FIND, &ep, &state->pstorage_htab, 0);
> +		hsearch_r(name, NULL, ENV_FIND, &ep, &state->pstorage_htab, 0);
>  		if (!ep)
>  			return TEE_ERROR_ITEM_NOT_FOUND;
>  
> @@ -196,15 +194,12 @@ static u32 ta_avb_invoke_func(struct udevice *dev, u32 func, uint num_params,
>  		value = params[1].u.memref.shm->addr;
>  		value_sz = params[1].u.memref.size;
>  
> -		e.key = name;
> -		e.data = NULL;
> -		hsearch_r(e, ENV_FIND, &ep, &state->pstorage_htab, 0);
> +		hsearch_r(name, NULL, ENV_FIND, &ep, &state->pstorage_htab, 0);
>  		if (ep)
> -			hdelete_r(e.key, &state->pstorage_htab, 0);
> +			hdelete_r(name, &state->pstorage_htab, 0);
>  
> -		e.key = name;
> -		e.data = value;
> -		hsearch_r(e, ENV_ENTER, &ep, &state->pstorage_htab, 0);
> +		hsearch_r(name, value, ENV_ENTER, &ep, &state->pstorage_htab,
> +			  0);
>  		if (!ep)
>  			return TEE_ERROR_OUT_OF_MEMORY;
>  
> diff --git a/env/callback.c b/env/callback.c
> index f0904cfdc5..e2296f9c5e 100644
> --- a/env/callback.c
> +++ b/env/callback.c
> @@ -92,13 +92,10 @@ static int clear_callback(struct env_entry *entry)
>   */
>  static int set_callback(const char *name, const char *value, void *priv)
>  {
> -	struct env_entry e, *ep;
> +	struct env_entry *ep;
>  	struct env_clbk_tbl *clbkp;
>  
> -	e.key	= name;
> -	e.data	= NULL;
> -	e.callback = NULL;
> -	hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
> +	hsearch_r(name, NULL, ENV_FIND, &ep, &env_htab, 0);
>  
>  	/* does the env variable actually exist? */
>  	if (ep != NULL) {
> diff --git a/env/flags.c b/env/flags.c
> index 418d6cc742..bc2348e1d2 100644
> --- a/env/flags.c
> +++ b/env/flags.c
> @@ -453,12 +453,9 @@ static int clear_flags(struct env_entry *entry)
>   */
>  static int set_flags(const char *name, const char *value, void *priv)
>  {
> -	struct env_entry e, *ep;
> +	struct env_entry *ep;
>  
> -	e.key	= name;
> -	e.data	= NULL;
> -	e.callback = NULL;
> -	hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
> +	hsearch_r(name, NULL, ENV_FIND, &ep, &env_htab, 0);
>  
>  	/* does the env variable actually exist? */
>  	if (ep != NULL) {
> diff --git a/include/search.h b/include/search.h
> index 0469a852e0..7243327f44 100644
> --- a/include/search.h
> +++ b/include/search.h
> @@ -68,7 +68,7 @@ void hdestroy_r(struct hsearch_data *htab);
>   * NULL.  If action is `ENV_ENTER' replace existing data (if any) with
>   * item.data.
>   * */
> -int hsearch_r(struct env_entry item, enum env_action action,
> +int hsearch_r(const char *key, const char *data, enum env_action action,
>  	      struct env_entry **retval, struct hsearch_data *htab, int flag);
>  
>  /*
> diff --git a/lib/hashtable.c b/lib/hashtable.c
> index 2caab0a4c6..d91f0d75e8 100644
> --- a/lib/hashtable.c
> +++ b/lib/hashtable.c
> @@ -225,21 +225,24 @@ int hmatch_r(const char *match, int last_idx, struct env_entry **retval,
>   * Compare an existing entry with the desired key, and overwrite if the action
>   * is ENV_ENTER.  This is simply a helper function for hsearch_r().
>   */
> -static inline int _compare_and_overwrite_entry(struct env_entry item,
> -		enum env_action action, struct env_entry **retval,
> -		struct hsearch_data *htab, int flag, unsigned int hval,
> -		unsigned int idx)
> +static inline int _compare_and_overwrite_entry(const char *key,
> +					       const char *data,
> +					       enum env_action action,
> +					       struct env_entry **retval,
> +					       struct hsearch_data *htab,
> +					       int flag, unsigned int hval,
> +					       unsigned int idx)
>  {
> -	if (htab->table[idx].used == hval
> -	    && strcmp(item.key, htab->table[idx].entry.key) == 0) {
> +	if (htab->table[idx].used == hval &&
> +	    strcmp(key, htab->table[idx].entry.key) == 0) {
>  		/* Overwrite existing value? */
> -		if (action == ENV_ENTER && item.data) {
> +		if (action == ENV_ENTER && data) {
>  			/* check for permission */
>  			if (htab->change_ok != NULL && htab->change_ok(
> -			    &htab->table[idx].entry, item.data,
> +			    &htab->table[idx].entry, data,
>  			    env_op_overwrite, flag)) {
> -				debug("change_ok() rejected setting variable "
> -					"%s, skipping it!\n", item.key);
> +				debug("change_ok() rejected setting variable %s, skipping it!\n",
> +				      key);
>  				__set_errno(EPERM);
>  				*retval = NULL;
>  				return 0;
> @@ -247,17 +250,17 @@ static inline int _compare_and_overwrite_entry(struct env_entry item,
>  
>  			/* If there is a callback, call it */
>  			if (htab->table[idx].entry.callback &&
> -			    htab->table[idx].entry.callback(item.key,
> -			    item.data, env_op_overwrite, flag)) {
> -				debug("callback() rejected setting variable "
> -					"%s, skipping it!\n", item.key);
> +			    htab->table[idx].entry.callback(key,
> +			    data, env_op_overwrite, flag)) {
> +				debug("callback() rejected setting variable %s, skipping it!\n",
> +				      key);
>  				__set_errno(EINVAL);
>  				*retval = NULL;
>  				return 0;
>  			}
>  
>  			free(htab->table[idx].entry.data);
> -			htab->table[idx].entry.data = strdup(item.data);
> +			htab->table[idx].entry.data = strdup(data);
>  			if (!htab->table[idx].entry.data) {
>  				__set_errno(ENOMEM);
>  				*retval = NULL;
> @@ -272,12 +275,12 @@ static inline int _compare_and_overwrite_entry(struct env_entry item,
>  	return -1;
>  }
>  
> -int hsearch_r(struct env_entry item, enum env_action action,
> +int hsearch_r(const char *key, const char *data, enum env_action action,
>  	      struct env_entry **retval, struct hsearch_data *htab, int flag)
>  {
>  	unsigned int hval;
>  	unsigned int count;
> -	unsigned int len = strlen(item.key);
> +	unsigned int len = strlen(key);
>  	unsigned int idx;
>  	unsigned int first_deleted = 0;
>  	int ret;
> @@ -287,7 +290,7 @@ int hsearch_r(struct env_entry item, enum env_action action,
>  	count = len;
>  	while (count-- > 0) {
>  		hval <<= 4;
> -		hval += item.key[count];
> +		hval += key[count];
>  	}
>  
>  	/*
> @@ -312,8 +315,8 @@ int hsearch_r(struct env_entry item, enum env_action action,
>  		    && !first_deleted)
>  			first_deleted = idx;
>  
> -		ret = _compare_and_overwrite_entry(item, action, retval, htab,
> -			flag, hval, idx);
> +		ret = _compare_and_overwrite_entry(key, data, action, retval,
> +						   htab, flag, hval, idx);
>  		if (ret != -1)
>  			return ret;
>  
> @@ -345,8 +348,9 @@ int hsearch_r(struct env_entry item, enum env_action action,
>  				first_deleted = idx;
>  
>  			/* If entry is found use it. */
> -			ret = _compare_and_overwrite_entry(item, action, retval,
> -				htab, flag, hval, idx);
> +			ret = _compare_and_overwrite_entry(key, data, action,
> +							   retval, htab, flag,
> +							   hval, idx);
>  			if (ret != -1)
>  				return ret;
>  		}
> @@ -367,14 +371,14 @@ int hsearch_r(struct env_entry item, enum env_action action,
>  
>  		/*
>  		 * Create new entry;
> -		 * create copies of item.key and item.data
> +		 * create copies of key and data
>  		 */
>  		if (first_deleted)
>  			idx = first_deleted;
>  
>  		htab->table[idx].used = hval;
> -		htab->table[idx].entry.key = strdup(item.key);
> -		htab->table[idx].entry.data = strdup(item.data);
> +		htab->table[idx].entry.key = strdup(key);
> +		htab->table[idx].entry.data = strdup(data);
>  		if (!htab->table[idx].entry.key ||
>  		    !htab->table[idx].entry.data) {
>  			__set_errno(ENOMEM);
> @@ -391,10 +395,10 @@ int hsearch_r(struct env_entry item, enum env_action action,
>  
>  		/* check for permission */
>  		if (htab->change_ok != NULL && htab->change_ok(
> -		    &htab->table[idx].entry, item.data, env_op_create, flag)) {
> -			debug("change_ok() rejected setting variable "
> -				"%s, skipping it!\n", item.key);
> -			_hdelete(item.key, htab, &htab->table[idx].entry, idx);
> +		    &htab->table[idx].entry, data, env_op_create, flag)) {
> +			debug("change_ok() rejected setting variable %s, skipping it!\n",
> +			      key);
> +			_hdelete(key, htab, &htab->table[idx].entry, idx);
>  			__set_errno(EPERM);
>  			*retval = NULL;
>  			return 0;
> @@ -402,11 +406,11 @@ int hsearch_r(struct env_entry item, enum env_action action,
>  
>  		/* If there is a callback, call it */
>  		if (htab->table[idx].entry.callback &&
> -		    htab->table[idx].entry.callback(item.key, item.data,
> +		    htab->table[idx].entry.callback(key, data,
>  		    env_op_create, flag)) {
> -			debug("callback() rejected setting variable "
> -				"%s, skipping it!\n", item.key);
> -			_hdelete(item.key, htab, &htab->table[idx].entry, idx);
> +			debug("callback() rejected setting variable %s, skipping it!\n",
> +			      key);
> +			_hdelete(key, htab, &htab->table[idx].entry, idx);
>  			__set_errno(EINVAL);
>  			*retval = NULL;
>  			return 0;
> @@ -449,14 +453,12 @@ static void _hdelete(const char *key, struct hsearch_data *htab,
>  
>  int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
>  {
> -	struct env_entry e, *ep;
> +	struct env_entry *ep;
>  	int idx;
>  
>  	debug("hdelete: DELETE key \"%s\"\n", key);
>  
> -	e.key = (char *)key;
> -
> -	idx = hsearch_r(e, ENV_FIND, &ep, htab, 0);
> +	idx = hsearch_r(key, NULL, ENV_FIND, &ep, htab, 0);
>  	if (idx == 0) {
>  		__set_errno(ESRCH);
>  		return 0;	/* not found */
> @@ -871,7 +873,7 @@ int himport_r(struct hsearch_data *htab,
>  	}
>  	/* Parse environment; allow for '\0' and 'sep' as separators */
>  	do {
> -		struct env_entry e, *rv;
> +		struct env_entry *rv;
>  
>  		/* skip leading white space */
>  		while (isblank(*dp))
> @@ -928,10 +930,7 @@ int himport_r(struct hsearch_data *htab,
>  			continue;
>  
>  		/* enter into hash table */
> -		e.key = name;
> -		e.data = value;
> -
> -		hsearch_r(e, ENV_ENTER, &rv, htab, flag);
> +		hsearch_r(name, value, ENV_ENTER, &rv, htab, flag);
>  		if (rv == NULL)
>  			printf("himport_r: can't insert \"%s=%s\" into hash table\n",
>  				name, value);
> diff --git a/test/env/hashtable.c b/test/env/hashtable.c
> index 5242c4cc3e..36a59d859a 100644
> --- a/test/env/hashtable.c
> +++ b/test/env/hashtable.c
> @@ -18,17 +18,12 @@ static int htab_fill(struct unit_test_state *uts,
>  		     struct hsearch_data *htab, size_t size)
>  {
>  	size_t i;
> -	struct env_entry item;
>  	struct env_entry *ritem;
>  	char key[20];
>  
>  	for (i = 0; i < size; i++) {
>  		sprintf(key, "%d", (int)i);
> -		item.callback = NULL;
> -		item.data = key;
> -		item.flags = 0;
> -		item.key = key;
> -		ut_asserteq(1, hsearch_r(item, ENV_ENTER, &ritem, htab, 0));
> +		ut_asserteq(1, hsearch_r(key, key, ENV_ENTER, &ritem, htab, 0));
>  	}
>  
>  	return 0;
> @@ -38,17 +33,12 @@ static int htab_check_fill(struct unit_test_state *uts,
>  			   struct hsearch_data *htab, size_t size)
>  {
>  	size_t i;
> -	struct env_entry item;
>  	struct env_entry *ritem;
>  	char key[20];
>  
>  	for (i = 0; i < size; i++) {
>  		sprintf(key, "%d", (int)i);
> -		item.callback = NULL;
> -		item.flags = 0;
> -		item.data = key;
> -		item.key = key;
> -		hsearch_r(item, ENV_FIND, &ritem, htab, 0);
> +		hsearch_r(key, key, ENV_FIND, &ritem, htab, 0);
>  		ut_assert(ritem);
>  		ut_asserteq_str(key, ritem->key);
>  		ut_asserteq_str(key, ritem->data);
> @@ -61,20 +51,15 @@ static int htab_create_delete(struct unit_test_state *uts,
>  			      struct hsearch_data *htab, size_t iterations)
>  {
>  	size_t i;
> -	struct env_entry item;
>  	struct env_entry *ritem;
>  	char key[20];
>  
>  	for (i = 0; i < iterations; i++) {
>  		sprintf(key, "cd-%d", (int)i);
> -		item.callback = NULL;
> -		item.flags = 0;
> -		item.data = key;
> -		item.key = key;
> -		hsearch_r(item, ENV_ENTER, &ritem, htab, 0);
> +		hsearch_r(key, key, ENV_ENTER, &ritem, htab, 0);
>  		ritem = NULL;
>  
> -		hsearch_r(item, ENV_FIND, &ritem, htab, 0);
> +		hsearch_r(key, key, ENV_FIND, &ritem, htab, 0);
>  		ut_assert(ritem);
>  		ut_asserteq_str(key, ritem->key);
>  		ut_asserteq_str(key, ritem->data);
> -- 
> 2.24.0
>

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

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

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

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 <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@denx.de>
> 
> Best regards,
> 
> Wolfgang Denk
>

Dear Simon,

In message <11b25950-50a3-c9cd-3c2d-5b2d6583046b@gmail.com> you wrote:
>
> Hm, U-Boot's 'hsearch_r' does not conform to this 'standard' since 
> December 2012, see these 2 commits from 2012 and 2019:

Argh.  What shall I say... this should get fixed ?

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

Indeed.  I'm pretty sure I did not notice these.

Thanks!

Best regards,

Wolfgang Denk