Message ID | 0102016e8e613d09-6624acd0-7882-4283-8512-d56fc73489ed-000000@eu-west-1.amazonses.com |
---|---|
State | Deferred |
Headers | show |
Series | [U-Boot,v2,1/3] tools: checkpatch: Restore 'debug' and 'printf' to logFunctions list | expand |
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
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);
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(-)