diff mbox

[U-Boot,RFC] env: add command to set individual variables to default

Message ID 1315471768-21608-1-git-send-email-gerlando.falauto@keymile.com
State Changes Requested
Headers show

Commit Message

Gerlando Falauto Sept. 8, 2011, 8:49 a.m. UTC
Here I am proposing an implementation for setting individual variables
to their default values as outlined in
http://www.denx.de/wiki/U-Boot/TaskSetEnvironmentDefaults

For instance, to reset defautl values for variables bootcmd and bootdelay:
=> env default bootcmd bootdelay

There are a few issues which are not fully covered.
[In braces I put the behavior of this patch].

1) Previous implementation of “env default” only worked for the whole
environment, and “-f” was necessary in order to prevent the unexperienced
user from messing up the environment by just typing command names.
Should this behavior be kept? [Yes]

2) When a variable is not defined in the default environment, should
it get deleted (if existing)? [Yes] Should a warning be issued? [No]
What if it is neither defined in the current nor in the default env?

3) Should it be possible to disable this feature (i.e. to save space)?
[No]
Any suggestions for a meaningful configuration token?
I was thinking of CONFIG_RESTORE_INDIVIDUAL_ENV_VARS.

4) This implementation comes mostly from himport_r().
It crossed my mind that it might also be useful to *import*
individual variables from a file (rather that the default env).

For instance:
env import [flags] addr size vars...

Would only import variables "vars" and ignore all others.

With this idea in mind I kept all himport_r()'s support for external files
(i.e., comments, separator other than '\0', quoting).
If no one else envisions any use for such import commands, the code could
obviously be much smaller and simpler.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Signed-off-by: Holger Brunck <holger.brunck@keymile.com>
cc: Wolfgang Denk <wd@denx.de>
---
 common/cmd_nvedit.c   |   12 +++++-
 common/env_common.c   |  106 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/environment.h |    3 +
 3 files changed, 119 insertions(+), 2 deletions(-)

Comments

Mike Frysinger Sept. 9, 2011, 12:31 a.m. UTC | #1
On Thursday, September 08, 2011 04:49:28 Gerlando Falauto wrote:
> Here I am proposing an implementation for setting individual variables
> to their default values as outlined in
> http://www.denx.de/wiki/U-Boot/TaskSetEnvironmentDefaults
> 
> For instance, to reset defautl values for variables bootcmd and bootdelay:
> => env default bootcmd bootdelay

please put this behind a CONFIG so it can be turned off.  i have no preference 
for the default (defining it in config_defaults.h).
-mike
Wolfgang Denk Oct. 6, 2011, 9:39 p.m. UTC | #2
Dear Gerlando Falauto,

In message <1315471768-21608-1-git-send-email-gerlando.falauto@keymile.com> you wrote:
> Here I am proposing an implementation for setting individual variables
> to their default values as outlined in
> http://www.denx.de/wiki/U-Boot/TaskSetEnvironmentDefaults
> 
> For instance, to reset defautl values for variables bootcmd and bootdelay:
> => env default bootcmd bootdelay
> 
> There are a few issues which are not fully covered.
> [In braces I put the behavior of this patch].
> 
> 1) Previous implementation of “env default” only worked for the whole
> environment, and “-f” was necessary in order to prevent the unexperienced
> user from messing up the environment by just typing command names.
> Should this behavior be kept? [Yes]
> 
> 2) When a variable is not defined in the default environment, should
> it get deleted (if existing)? [Yes] Should a warning be issued? [No]
> What if it is neither defined in the current nor in the default env?
> 
> 3) Should it be possible to disable this feature (i.e. to save space)?
> [No]
> Any suggestions for a meaningful configuration token?
> I was thinking of CONFIG_RESTORE_INDIVIDUAL_ENV_VARS.
> 
> 4) This implementation comes mostly from himport_r().
> It crossed my mind that it might also be useful to *import*
> individual variables from a file (rather that the default env).
> 
> For instance:
> env import [flags] addr size vars...
> 
> Would only import variables "vars" and ignore all others.
> 
> With this idea in mind I kept all himport_r()'s support for external files
> (i.e., comments, separator other than '\0', quoting).
> If no one else envisions any use for such import commands, the code could
> obviously be much smaller and simpler.
> 
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> Signed-off-by: Holger Brunck <holger.brunck@keymile.com>
> cc: Wolfgang Denk <wd@denx.de>
> ---
>  common/cmd_nvedit.c   |   12 +++++-
>  common/env_common.c   |  106 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/environment.h |    3 +
>  3 files changed, 119 insertions(+), 2 deletions(-)

Checkpatch says:

total: 5 errors, 0 warnings, 147 lines checked

Please clean up and resubmit.  Thanks.

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index b2c88ba..9822785 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -580,10 +580,17 @@  int envmatch(uchar *s1, int i2)
 
 static int do_env_default(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	if ((argc != 2) || (strcmp(argv[1], "-f") != 0))
+	/* Check that we have at least one argument */
+	if (argc < 2) {
 		return cmd_usage(cmdtp);
+	} else if ((argc == 2) && (strcmp(argv[1], "-f")) == 0) {
+		/* Reset the whole environment */
+		set_default_env("## Resetting to default environment\n");
+		return 0;
+	}
 
-	set_default_env("## Resetting to default environment\n");
+	/* Reset individual variables */
+	env_default_vars(argc-1, argv+1);
 	return 0;
 }
 
@@ -910,6 +917,7 @@  U_BOOT_CMD(
 	"ask name [message] [size] - ask for environment variable\nenv "
 #endif
 	"default -f - reset default environment\n"
+	"env default name [...] - reset variable(s) to their default value\n"
 #if defined(CONFIG_CMD_EDITENV)
 	"env edit name - edit environment variable\n"
 #endif
diff --git a/common/env_common.c b/common/env_common.c
index c3e6388..b2d68f8 100644
--- a/common/env_common.c
+++ b/common/env_common.c
@@ -197,6 +197,112 @@  void set_default_env(const char *s)
 }
 
 /*
+ * import individual variables from an external environment
+ * (e.g. default environment). 
+ * Most of this code comes straight from himport_r(). 
+ */
+static int env_import_vars(const char *env, const size_t size, const char sep,
+			   int nvars, char * const vars[])
+{
+	char *data, *sp, *dp, *name, *value, *thisvalue;
+	int i;
+
+	/* we allocate new space to make sure we can write to the array */
+	data = malloc(size);
+	if (data == NULL) {
+		debug("env_default_vars: can't malloc %d bytes\n", size);
+		__set_errno(ENOMEM);
+		return 0;
+	}
+
+	/* Loop through all passed variables */
+	for (i = 0; i < nvars; i++) {
+		debug("looking for a default value for %s\n", vars[i]);
+
+		memcpy(data, env, size);
+		dp = data;
+
+		/* 
+		 * Unless proven otherwise, this variable
+		 * does not exist in the default env
+		 */
+		thisvalue = "";
+		/* Parse environment; allow for '\0' and 'sep' as separators */
+		do {
+			/* skip leading white space */
+			while ((*dp == ' ') || (*dp == '\t'))
+				++dp;
+
+			/* skip comment lines */
+			if (*dp == '#') {
+				while (*dp && (*dp != sep))
+					++dp;
+				++dp;
+				continue;
+			}
+
+			/* parse name */
+			for (name = dp; *dp != '=' && *dp && *dp != sep; ++dp)
+				;
+
+			/* deal with "name" and "name=" entries (delete var) */
+			if (*dp == '\0' || *(dp + 1) == '\0' ||
+				*dp == sep || *(dp + 1) == sep) {
+				if (*dp == '=')
+					*dp++ = '\0';
+				*dp++ = '\0';	/* terminate name */
+
+				/* default is none */
+				value = "";
+			} else {
+				*dp++ = '\0';	/* terminate name */
+				value = dp; /* value starts here */
+				/* parse value; deal with escapes */
+				for (sp = dp; *dp && (*dp != sep); ++dp) {
+					if ((*dp == '\\') && *(dp + 1))
+						++dp;
+					*sp++ = *dp;
+				}
+				*sp++ = '\0';	/* terminate value */
+			}
+
+			if (strcmp(name, vars[i]) == 0) {
+				debug("found variable: %s\n"
+				      "default value: %s\n", name, value);
+				thisvalue = value;
+				/* exit from loop parsing the default env */
+				break;
+			}
+			++dp;
+
+		} while ((dp < data + size) && *dp);
+		/*
+		 * size check needed for text without '\0' termination 
+		 * (e.g. default environment) 
+		 */
+
+		debug("setting default value: %s=%s\n", vars[i], thisvalue);
+		setenv(vars[i], thisvalue);
+	} /* for-loop over i */
+
+	debug("env_default_vars: free(data = %p)\n", data);
+	free(data);
+	debug("env_default_vars: done\n");
+	return 1;		/* everything OK */
+
+}
+
+/* [re]set individual variables to their value in the default environment */
+int env_default_vars(int nvars, char * const vars[])
+{
+	/* Special use-case: import from default environment
+	   (and use \0 as a separator) */
+	return env_import_vars((const char *)default_environment,
+			       sizeof(default_environment), '\0',
+			       nvars, vars);
+}
+
+/*
  * Check if CRC is valid and (if yes) import the environment.
  * Note that "buf" may or may not be aligned.
  */
diff --git a/include/environment.h b/include/environment.h
index 53d92df..2c4d905 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -169,6 +169,9 @@  void env_crc_update (void);
 /* [re]set to the default environment */
 void set_default_env(const char *s);
 
+/* [re]set individual variables to their value in the default environment */
+int env_default_vars(int nvars, char * const vars[]);
+
 /* Import from binary representation into hash table */
 int env_import(const char *buf, int check);