[U-Boot] [PATCH v2 2/3] env: Tidy up some of the env code
James Byrne
james.byrne at origamienergy.com
Thu Nov 21 14:32:47 UTC 2019
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 at 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
More information about the U-Boot
mailing list