Re: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes

Dear Gerlando Falauto,

WD prodded me too long to review this patchset ;-D

The logic of checking special parameters (e.g. baudrate, stdin, stdout, for a valid value and/or whether can be overwritten) and applying the new value to the running system is now all within a single function env_check_apply() which can be called whenever changes are made to the environment, no matter if by set, default or import.

With this patch env_check_apply() is only called by "env set", retaining previous behavior.

Also allow for selectively importing/resetting variables.

So add 3 new arguments to himport_r(): o "nvars", "vars":, number and list of variables to take into account (0 means ALL)

o "apply" callback function to check whether a variable can be overwritten, and possibly immediately apply the changes; when NULL, no check is performed.

Signed-off-by: Gerlando Falauto gerlando.falauto@keymile.com

[...]

• }

+#if defined(CONFIG_CMD_NET)

• else if (strcmp(name, "bootfile") == 0) {

• copy_filename(BootFile, newval, sizeof(BootFile));
  

Can you remove the camel-case here please?

• return 0;
  

• }

+#endif

• return 0;

+}

+/*

• • Set a new environment variable,
• • or replace or delete an existing one.

+*/ +int _do_env_set(int flag, int argc, char * const argv[]) +{

• int i, len;
• char *name, *value, *s;
• ENTRY e, *ep;
• name = argv[1];
• value = argv[2];
• if (strchr(name, '=')) {

• printf("## Error: illegal character '='"
  

•        "in variable name \"%s\"\n", name);
  

• return 1;
  

• }
• env_id++;
• /*

• * search if variable with this name already exists
  

• */
  

• e.key = name;
• e.data = NULL;
• hsearch_r(e, FIND, &ep, &env_htab);
• /*

• * Perform requested checks. Notice how since we are overwriting
  

• * a single variable, we need to set H_NOCLEAR
  

• */
  

• if (env_check_apply(name, ep ? ep->data : NULL, value, H_NOCLEAR)) {

• debug("check function did not approve, refusing\n");
  

• return 1;
  

• }
• /* Delete only ? */ if (argc < 3 || argv[2] == NULL) { int rc = hdelete_r(name, &env_htab);

@@ -337,34 +412,6 @@ int _do_env_set(int flag, int argc, char * const argv[]) return 1; }

• /*

• * Some variables should be updated when the corresponding
  

• * entry in the environment is changed
  

• */
  

• if (strcmp(name, "ipaddr") == 0) {

• char *s = argv[2];	/* always use only one arg */
  

• char *e;
  

• unsigned long addr;
  

• bd->bi_ip_addr = 0;
  

• for (addr = 0, i = 0; i < 4; ++i) {
  

• 	ulong val = s ? simple_strtoul(s, &e, 10) : 0;
  

• 	addr <<= 8;
  

• 	addr  |= val & 0xFF;
  

• 	if (s)
  

• 		s = *e ? e + 1 : e;
  

• }
  

• bd->bi_ip_addr = htonl(addr);
  

• return 0;
  

• } else if (strcmp(argv[1], "loadaddr") == 0) {

• load_addr = simple_strtoul(argv[2], NULL, 16);
  

• return 0;
  

• }

-#if defined(CONFIG_CMD_NET)

• else if (strcmp(argv[1], "bootfile") == 0) {

• copy_filename(BootFile, argv[2], sizeof(BootFile));
  

• return 0;
  

• }

-#endif return 0; }

@@ -886,7 +933,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, addr = (char *)ep->data; }

• if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) {

• if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR,

• 	0, NULL, NULL) == 0) {
  

  error("Environment import failed: errno = %d\n", errno); return 1; }

diff --git a/common/env_common.c b/common/env_common.c index 8a71096..7e2bb2f 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -175,7 +175,8 @@ void set_default_env(const char *s) }

if (himport_r(&env_htab, (char *)default_environment,

• 	sizeof(default_environment), '\0', 0) == 0)
  

• 	sizeof(default_environment), '\0', 0,
  

• 	0, NULL, NULL) == 0)
  

  error("Environment import failed: errno = %d\n", errno);

  gd->flags |= GD_FLG_ENV_READY;

@@ -200,7 +201,8 @@ int env_import(const char *buf, int check) } }

• if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) {

• if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0,

• 	0, NULL, NULL)) {
  

  gd->flags |= GD_FLG_ENV_READY; return 1; }

diff --git a/include/environment.h b/include/environment.h index 3c145af..3a3e6b8 100644 --- a/include/environment.h +++ b/include/environment.h @@ -193,6 +193,13 @@ void set_default_env(const char *s); /* Import from binary representation into hash table */ int env_import(const char *buf, int check);

+/*

• • Check if variable "name" can be changed from oldval to newval,
• • and if so, apply the changes (e.g. baudrate)
• */

+int env_check_apply(const char *name, const char *oldval,

• 	const char *newval, int flag);
  

#endif /* DO_DEPS_ONLY */

#endif /* _ENVIRONMENT_H_ */ diff --git a/include/search.h b/include/search.h index ef53edb..2a59e03 100644 --- a/include/search.h +++ b/include/search.h @@ -47,6 +47,13 @@ typedef struct entry { struct _ENTRY;

/*

• • Callback function to be called for checking whether the given change

may + * be applied or not. Must return 0 for approval, 1 for denial.

• */

+typedef int (*apply_cb)(const char *name, const char *oldval,

• 	const char *newval, int flag);
  

Is the typedef really necessary ?

+/*

• Family of hash table handling functions. The functions also
• have reentrant counterparts ending with _r. The non-reentrant
• functions all work on a signle internal hashing table.

@@ -94,11 +101,19 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, const char __sep, char **__resp, size_t __size, int argc, char * const argv[]);

+/*

• • nvars, vars: variables to import (nvars == 0 means all)
• • apply_cb: callback function to check validity of the new argument,
• • and possibly apply changes (NULL means accept everything)
• */

extern int himport_r(struct hsearch_data *__htab, const char *__env, size_t __size, const char __sep,

•      int __flag);
  

•      int __flag,
  

•      int nvars, char * const vars[],
  

•      apply_cb apply);
  
  

/* Flags for himport_r() */ #define H_NOCLEAR 1 /* do not clear hash table before

importing */

+#define H_FORCE 2 /* overwrite read-only/write-once

variables */

Make this 1 << x please.

#endif /* search.h */ diff --git a/lib/hashtable.c b/lib/hashtable.c index abd61c8..75b9b07 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, * himport() */

+/* Check whether variable name is amongst vars[] */ +static int process_var(const char *name, int nvars, char * const vars[])

You mean check_var()?

+{

• int i = 0;
• /* No variables specified means process all of them */
• if (nvars == 0)

• return 1;
  

• for (i = 0; i < nvars; i++) {

• if (!strcmp(name, vars[i]))
  

• 	return 1;
  

• }
• debug("Skipping non-listed variable %s\n", name);
• return 0;

+}

/*

• Import linearized data into hash table.

@@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, */

int himport_r(struct hsearch_data *htab,

•      const char *env, size_t size, const char sep, int flag)
  

• const char *env, size_t size, const char sep, int flag,
  

• int nvars, char * const vars[],
  

• apply_cb apply)
  

{ char *data, *sp, *dp, *name, *value;

@@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab, *dp++ = '\0'; /* terminate name */

	debug("DELETE CANDIDATE: \"%s\"\n", name);

• 	if (!process_var(name, nvars, vars))
  

• 		continue;
  
  if (hdelete_r(name, htab) == 0)
  	debug("DELETE ERROR 
  

##############################\n");

@@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab, *sp++ = '\0'; /* terminate value */ ++dp;

• /* Skip variables which are not supposed to be treated */
  

• if (!process_var(name, nvars, vars))
  

• 	continue;
  

• /* enter into hash table */ e.key = name; e.data = value;

Do you need to do this if you eventually later figure out you have no apply() callback and you did this for no reason?

• /* if there is an apply function, check what it has to say */
  

• if (apply != NULL) {
  

• 	debug("searching before calling cb function"
  

• 		" for  %s\n", name);
  

• 	/*
  

• 	 * Search for variable in existing env, so to pass
  

• 	 * its previous value to the apply callback
  

• 	 */
  

• 	hsearch_r(e, FIND, &rv, htab);
  

• 	debug("previous value was %s\n", rv ? rv->data : "");
  

• 	if (apply(name, rv ? rv->data : NULL, value, flag)) {
  

• 		debug("callback function refused to set"
  

• 			" variable %s, skipping it!\n", name);
  

• 		continue;
  

• 	}
  

• }
  

• hsearch_r(e, ENTER, &rv, htab); if (rv == NULL) { printf("himport_r: can't insert "%s=%s" into hash

table\n",