[U-Boot] [PATCH v2 0/4] tools/env: minor refactorings

[U-Boot] [PATCH v2 0/4] tools/env: minor refactorings

[Andreas Fenkart]

various patches, see individual patch descriptions

Andreas Fenkart (4):
  tools/env: return with error if redundant environments have unequal
    size
  tools/env: kernel-doc for fw_printenv, fw_getenv and fw_parse_script
  tools/env: move envmatch further up in file to avoid forward
    declarations
  tools/env: reuse fw_getenv in fw_printenv function

 tools/env/fw_env.c | 107 ++++++++++++++++++++++---------------------------
 tools/env/fw_env.h | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 162 insertions(+), 60 deletions(-)

-- 
2.8.1

[U-Boot] [PATCH v2 1/4] tools/env: return with error if redundant environments have unequal size

[Andreas Fenkart]

For double buffering to work, the target buffer must always be big
enough to hold all data. This can only be ensured if buffers are of
equal size, otherwise one must be smaller and we risk data loss
when copying from the bigger to the smaller buffer.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 692abda..b1c8217 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1382,18 +1382,19 @@ static int parse_config(struct env_opts *opts)
 		return -1;
 	}
 
-	if (HaveRedundEnv && stat (DEVNAME (1), &st)) {
-		fprintf (stderr,
-			"Cannot access MTD device %s: %s\n",
-			DEVNAME (1), strerror (errno));
-		return -1;
-	}
+	if (HaveRedundEnv) {
+		if (stat(DEVNAME(1), &st)) {
+			fprintf(stderr,
+				"Cannot access MTD device %s: %s\n",
+				DEVNAME(1), strerror(errno));
+			return -1;
+		}
 
-	if (HaveRedundEnv && ENVSIZE(0) != ENVSIZE(1)) {
-		ENVSIZE(0) = ENVSIZE(1) = min(ENVSIZE(0), ENVSIZE(1));
-		fprintf(stderr,
-			"Redundant environments have inequal size, set to 0x%08lx\n",
-			ENVSIZE(1));
+		if (ENVSIZE(0) != ENVSIZE(1)) {
+			fprintf(stderr,
+				"Redundant environments have unequal size");
+			return -1;
+		}
 	}
 
 	usable_envsize = CUR_ENVSIZE - sizeof(uint32_t);
-- 
2.8.1

[U-Boot] [PATCH v2 2/4] tools/env: kernel-doc for fw_printenv, fw_getenv and fw_parse_script

[Andreas Fenkart]

there are two groups of functions:
- application ready tools: fw_setenv/fw_getenv/fw_parse_script
these are used, when creating a single binary containing multiple
tools (busybox like)
- file access like: open/read/write/close
above functions are implemented on top of these. applications
can use those to modify several variables without creating a
temporary batch script file
tested with "./scripts/kernel-doc -html -v tools/env/fw_env.h"

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c |   2 +-
 tools/env/fw_env.h | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 115 insertions(+), 2 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index b1c8217..fe479e9 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -483,7 +483,7 @@ int fw_setenv(int argc, char *argv[], struct env_opts *opts)
 	valc = argc - 1;
 
 	if (env_flags_validate_env_set_params(name, valv, valc) < 0)
-		return 1;
+		return -1;
 
 	len = 0;
 	for (i = 0; i < valc; ++i) {
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
index dac964d..436eca9 100644
--- a/tools/env/fw_env.h
+++ b/tools/env/fw_env.h
@@ -67,12 +67,125 @@ struct env_opts {
 
 int parse_aes_key(char *key, uint8_t *bin_key);
 
+/**
+ * fw_printenv() - print one or several environment variables
+ *
+ * @argc: number of variables names to be printed, prints all if 0
+ * @argv: array of variable names to be printed, if argc != 0
+ * @value_only: do not repeat the variable name, print the bare value,
+ *          only one variable allowed with this option, argc must be 1
+ * @opts: encryption key, configuration file, defaults are used if NULL
+ *
+ * Description:
+ *  Uses fw_env_open, fw_getenv
+ *
+ * Return:
+ *  0 on success, -1 on failure (modifies errno)
+ */
 int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts);
-char *fw_getenv(char *name);
+
+/**
+ * fw_setenv() - adds or removes one variable to the environment
+ *
+ * @argc: number of strings in argv, argv[0] is variable name,
+ *          argc==1 means erase variable, argc > 1 means add a variable
+ * @argv: argv[0] is variable name, argv[1..argc-1] are concatenated separated
+ *           by single blank and set as the new value of the variable
+ * @opts: how to retrieve environment from flash, defaults are used if NULL
+ *
+ * Description:
+ *  Uses fw_env_open, fw_env_write, fw_env_close
+ *
+ * Return:
+ *  0 on success, -1 on failure (modifies errno)
+ *
+ * ERRORS:
+ *  EROFS - some variables ("ethaddr", "serial#") cannot be modified
+ */
 int fw_setenv(int argc, char *argv[], struct env_opts *opts);
+
+/**
+ * fw_parse_script() - adds or removes multiple variables with a batch script
+ *
+ * @fname: batch script file name
+ * @opts: encryption key, configuration file, defaults are used if NULL
+ *
+ * Description:
+ *  Uses fw_env_open, fw_env_write, fw_env_close
+ *
+ * Return:
+ *  0 success, -1 on failure (modifies errno)
+ *
+ * Script Syntax:
+ *
+ *  key [ [space]+ value]
+ *
+ *  lines starting with '#' treated as comment
+ *
+ *  A variable without value will be deleted. Any number of spaces are allowed
+ *  between key and value. The value starts with the first non-space character
+ *  and ends with newline. No comments allowed on these lines.  Spaces inside
+ *  the value are preserved verbatim.
+ *
+ * Script Example:
+ *
+ *  netdev         eth0
+ *
+ *  kernel_addr    400000
+ *
+ *  foo            spaces           are copied verbatim
+ *
+ *  # delete variable bar
+ *
+ *  bar
+ */
 int fw_parse_script(char *fname, struct env_opts *opts);
+
+
+/**
+ * fw_env_open() - read enviroment from flash into RAM cache
+ *
+ * @opts: encryption key, configuration file, defaults are used if NULL
+ *
+ * Return:
+ *  0 on success, -1 on failure (modifies errno)
+ */
 int fw_env_open(struct env_opts *opts);
+
+/**
+ * fw_getenv() - lookup variable in the RAM cache
+ *
+ * @name: variable to be searched
+ * Return:
+ *  pointer to start of value, NULL if not found
+ */
+char *fw_getenv(char *name);
+
+/**
+ * fw_env_write() - modify a variable held in the RAM cache
+ *
+ * @name: variable
+ * @value: delete variable if NULL, otherwise create or overwrite the variable
+ *
+ * This is called in sequence to update the environment in RAM without updating
+ * the copy in flash after each set
+ *
+ * Return:
+ *  0 on success, -1 on failure (modifies errno)
+ *
+ * ERRORS:
+ *  EROFS - some variables ("ethaddr", "serial#") cannot be modified
+ */
 int fw_env_write(char *name, char *value);
+
+/**
+ * fw_env_close - write the environment from RAM cache back to flash
+ *
+ * @opts: encryption key, configuration file, defaults are used if NULL
+ *
+ * Return:
+ *  0 on success, -1 on failure (modifies errno)
+ */
 int fw_env_close(struct env_opts *opts);
 
 unsigned long crc32(unsigned long, const unsigned char *, unsigned);
-- 
2.8.1

[U-Boot] [PATCH v2 3/4] tools/env: move envmatch further up in file to avoid forward declarations

[Andreas Fenkart]

forward declaration not needed when re-ordered

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index fe479e9..f155c12 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -121,7 +121,6 @@ static unsigned char obsolete_flag = 0;
 #include <env_default.h>
 
 static int flash_io (int mode);
-static char *envmatch (char * s1, char * s2);
 static int parse_config(struct env_opts *opts);
 
 #if defined(CONFIG_FILE)
@@ -147,6 +146,24 @@ static char *skip_blanks(char *s)
 }
 
 /*
+ * s1 is either a simple 'name', or a 'name=value' pair.
+ * s2 is a 'name=value' pair.
+ * If the names match, return the value of s2, else NULL.
+ */
+static char *envmatch(char *s1, char *s2)
+{
+	if (s1 == NULL || s2 == NULL)
+		return NULL;
+
+	while (*s1 == *s2++)
+		if (*s1++ == '=')
+			return s2;
+	if (*s1 == '\0' && *(s2 - 1) == '=')
+		return s2;
+	return NULL;
+}
+
+/**
  * Search the environment for a variable.
  * Return the value, if found, or NULL, if not found.
  */
@@ -1121,25 +1138,6 @@ exit:
 }
 
 /*
- * s1 is either a simple 'name', or a 'name=value' pair.
- * s2 is a 'name=value' pair.
- * If the names match, return the value of s2, else NULL.
- */
-
-static char *envmatch (char * s1, char * s2)
-{
-	if (s1 == NULL || s2 == NULL)
-		return NULL;
-
-	while (*s1 == *s2++)
-		if (*s1++ == '=')
-			return s2;
-	if (*s1 == '\0' && *(s2 - 1) == '=')
-		return s2;
-	return NULL;
-}
-
-/*
  * Prevent confusion if running from erased flash memory
  */
 int fw_env_open(struct env_opts *opts)
-- 
2.8.1

[U-Boot] [PATCH v2 4/4] tools/env: reuse fw_getenv in fw_printenv function

[Andreas Fenkart]

Try to avoid adhoc iteration of the environment. Reuse fw_getenv
to find the variables that should be printed. Only use open-coded
iteration when printing all variables.
For backwards compatibility, keep emitting a newline when
printing with value_only.

Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 44 +++++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index f155c12..8b3a132 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -249,9 +249,14 @@ int parse_aes_key(char *key, uint8_t *bin_key)
  */
 int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts)
 {
-	char *env, *nxt;
 	int i, rc = 0;
 
+	if (value_only && argc != 1) {
+		fprintf(stderr,
+			"## Error: `-n' option requires exactly one argument\n");
+		return -1;
+	}
+
 	if (!opts)
 		opts = &default_opts;
 
@@ -259,6 +264,7 @@ int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts)
 		return -1;
 
 	if (argc == 0) {		/* Print all env variables  */
+		char *env, *nxt;
 		for (env = environment.data; *env; env = nxt + 1) {
 			for (nxt = env; *nxt; ++nxt) {
 				if (nxt >= &environment.data[ENV_SIZE]) {
@@ -273,39 +279,23 @@ int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts)
 		return 0;
 	}
 
-	if (value_only && argc != 1) {
-		fprintf(stderr,
-			"## Error: `-n' option requires exactly one argument\n");
-		return -1;
-	}
-
-	for (i = 0; i < argc; ++i) {	/* print single env variables   */
+	for (i = 0; i < argc; ++i) {	/* print a subset of env variables */
 		char *name = argv[i];
 		char *val = NULL;
 
-		for (env = environment.data; *env; env = nxt + 1) {
-
-			for (nxt = env; *nxt; ++nxt) {
-				if (nxt >= &environment.data[ENV_SIZE]) {
-					fprintf (stderr, "## Error: "
-						"environment not terminated\n");
-					return -1;
-				}
-			}
-			val = envmatch (name, env);
-			if (val) {
-				if (!value_only) {
-					fputs (name, stdout);
-					putc ('=', stdout);
-				}
-				puts (val);
-				break;
-			}
-		}
+		val = fw_getenv(name);
 		if (!val) {
 			fprintf (stderr, "## Error: \"%s\" not defined\n", name);
 			rc = -1;
+			continue;
 		}
+
+		if (value_only) {
+			puts(val);
+			break;
+		}
+
+		printf("%s=%s\n", name, val);
 	}
 
 	return rc;
-- 
2.8.1

[U-Boot] [U-Boot, v2, 1/4] tools/env: return with error if redundant environments have unequal size

[Tom Rini]

On Sat, Jul 16, 2016 at 05:06:12PM +0200, Andreas Fenkart wrote:

> For double buffering to work, the target buffer must always be big
> enough to hold all data. This can only be ensured if buffers are of
> equal size, otherwise one must be smaller and we risk data loss
> when copying from the bigger to the smaller buffer.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

With https://patchwork.ozlabs.org/patch/648142/ applied this needs to be
reworked, and may not be applicable now, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/a5375252/attachment.sig>

[U-Boot] [U-Boot, v2, 2/4] tools/env: kernel-doc for fw_printenv, fw_getenv and fw_parse_script

[Tom Rini]

On Sat, Jul 16, 2016 at 05:06:13PM +0200, Andreas Fenkart wrote:

> there are two groups of functions:
> - application ready tools: fw_setenv/fw_getenv/fw_parse_script
> these are used, when creating a single binary containing multiple
> tools (busybox like)
> - file access like: open/read/write/close
> above functions are implemented on top of these. applications
> can use those to modify several variables without creating a
> temporary batch script file
> tested with "./scripts/kernel-doc -html -v tools/env/fw_env.h"
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/38c674ac/attachment.sig>

[U-Boot] [U-Boot, v2, 3/4] tools/env: move envmatch further up in file to avoid forward declarations

[Tom Rini]

On Sat, Jul 16, 2016 at 05:06:14PM +0200, Andreas Fenkart wrote:

> forward declaration not needed when re-ordered
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/5818ad51/attachment.sig>

[U-Boot] [U-Boot, v2, 4/4] tools/env: reuse fw_getenv in fw_printenv function

[Tom Rini]

On Sat, Jul 16, 2016 at 05:06:15PM +0200, Andreas Fenkart wrote:

> Try to avoid adhoc iteration of the environment. Reuse fw_getenv
> to find the variables that should be printed. Only use open-coded
> iteration when printing all variables.
> For backwards compatibility, keep emitting a newline when
> printing with value_only.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160722/704df3dc/attachment.sig>