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

Dear Gerlando Falauto,

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.

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

common/cmd_nvedit.c | 170 ++++++++++++++++++++++++++++++++------------------- include/search.h | 3 +- 2 files changed, 109 insertions(+), 64 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index 22f9821..e762e76 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -197,32 +197,21 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag, #endif

/*

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

• • Perform consistency checking before setting, replacing, or deleting an
• • environment variable, then (if successful) apply the changes to

internals so + * to make them effective. Code for this function was taken out of + * _do_env_set(), which now calls it instead.

• • Returns 0 in case of success, 1 in case of failure.
• • When (flag & H_FORCE) is set, do not print out any error message and

force + * overwriting of write-once variables. */ -int _do_env_set(int flag, int argc, char * const argv[])

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

• 	const char *newval, int flag)
  

{ bd_t *bd = gd->bd;

• int i, len;

• int i; int console = -1;

• char *name, *value, *s;

• ENTRY e, *ep;

• name = argv[1];

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

  /* Check for console redirection */ if (strcmp(name, "stdin") == 0)

@@ -233,60 +222,76 @@ int _do_env_set(int flag, int argc, char * const argv[]) console = stderr;

if (console != -1) {

• if (argc < 3) {		/* Cannot delete it! */
  

• 	printf("Can't delete \"%s\"\n", name);
  

• if ((newval == NULL) || (*newval == '\0')) {
  

• 	/* We cannot delete stdin/stdout/stderr */
  

• 	if ((flag & H_FORCE) == 0)
  

Given H_FORCE isn't set on any env var yet, won't this break bisectability?

• 		printf("Can't delete \"%s\"\n", name);
  return 1;
  

  }

#ifdef CONFIG_CONSOLE_MUX

• i = iomux_doenv(console, argv[2]);
  

• i = iomux_doenv(console, newval);
  

  if (i) return i;

#else /* Try assigning specified device */

• if (console_assign(console, argv[2]) < 0)
  

• if (console_assign(console, newval) < 0)
  return 1;
  
  

#ifdef CONFIG_SERIAL_MULTI

• if (serial_assign(argv[2]) < 0)
  

• if (serial_assign(newval) < 0)
  return 1;
  

#endif #endif /* CONFIG_CONSOLE_MUX */ }

/*

• * Some variables like "ethaddr" and "serial#" can be set only
  

• * once and cannot be deleted; also, "ver" is readonly.
  

• * Some variables like "ethaddr" and "serial#" can be set only once and
  

• * cannot be deleted, unless CONFIG_ENV_OVERWRITE is defined.
  

  */

• if (ep) { /* variable exists */

#ifndef CONFIG_ENV_OVERWRITE

• if (oldval != NULL && /* variable exists */

• (flag & H_FORCE) == 0) {	/* and we are not forced */
  

  if (strcmp(name, "serial#") == 0 || (strcmp(name, "ethaddr") == 0

#if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)

•      && strcmp(ep->data, MK_STR(CONFIG_ETHADDR)) != 0
  

•      && strcmp(oldval, MK_STR(CONFIG_ETHADDR)) != 0
  

#endif /* CONFIG_OVERWRITE_ETHADDR_ONCE && CONFIG_ETHADDR */ )) { printf("Can't overwrite "%s"\n", name); return 1; }

• }

#endif

• /*

• * When we change baudrate, or we are doing an env default -a
  

• * (which will erase all variables prior to calling this),
  

• * we want the baudrate to actually change - for real.
  

• */
  

• if (oldval != NULL || /* variable exists */

• (flag & H_NOCLEAR) == 0) {	/* or env is clear */
  

  /*
  • Switch to new baudrate if new baudrate is supported
  */ if (strcmp(name, "baudrate") == 0) {

• 	int baudrate = simple_strtoul(argv[2], NULL, 10);
  

• 	int baudrate = simple_strtoul(newval, NULL, 10);
  int i;
  for (i = 0; i < N_BAUDRATES; ++i) {
  	if (baudrate == baudrate_table[i])
  		break;
  }
  if (i == N_BAUDRATES) {
  

• 		printf("## Baudrate %d bps not supported\n",
  

• 			baudrate);
  

• 		if ((flag & H_FORCE) == 0)
  

• 			printf("## Baudrate %d bps not "
  

• 				"supported\n", baudrate);
  	return 1;
  }
  

• 	if (gd->baudrate == baudrate) {
  

• 		/* If unchanged, we just say it's OK */
  

• 		return 0;
  

• 	}
  printf("## Switch baudrate to %d bps and"
  

• 	       "press ENTER ...\n", baudrate);
  

• 		"press ENTER ...\n", baudrate);
  

What changed above?

	udelay(50000);
	gd->baudrate = baudrate;

#if defined(CONFIG_PPC) || defined(CONFIG_MCF52x2) @@ -300,6 +305,73 @@ int _do_env_set(int flag, int argc, char * const argv[]) } }

• /*

• * Some variables should be updated when the corresponding
  

• * entry in the environment is changed
  

• */
  

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

• const char *s = newval;
  

• 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(name, "loadaddr") == 0) {

• load_addr = simple_strtoul(newval, NULL, 16);
  

• return 0;
  

• }

+#if defined(CONFIG_CMD_NET)

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

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

• 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 ? */

Shouldn't delete-only be handled before env_check_apply() to make it faster?

if (argc < 3 || argv[2] == NULL) { int rc = hdelete_r(name, &env_htab); @@ -337,34 +409,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; }

diff --git a/include/search.h b/include/search.h index ef53edb..a4a5ef4 100644 --- a/include/search.h +++ b/include/search.h @@ -99,6 +99,7 @@ extern int himport_r(struct hsearch_data *__htab, int __flag);

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

importing */

+#define H_NOCLEAR (1 << 0) /* do not clear hash table before

importing */

+#define H_FORCE (1 << 1) /* overwrite read-only/write-once

variables */

#endif /* search.h */