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

[U-Boot] [PATCH 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: javadoc 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 | 105 ++++++++++++++++++++++++-----------------------------
 tools/env/fw_env.h |  75 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 121 insertions(+), 59 deletions(-)

-- 
2.8.1

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

[Andreas Fenkart]

From: Andreas Fenkart <afenkart@gmail.com>

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.

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 2/4] tools/env: javadoc 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

Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.h | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
index dac964d..8bf74f3 100644
--- a/tools/env/fw_env.h
+++ b/tools/env/fw_env.h
@@ -67,12 +67,85 @@ 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, print all if 0
+ * @argv  array of variable names to be printed
+ * @value_only  do not repeat the variable name, only print the value,
+ *          only one variable allowed with this option, argc must be 1
+ * @opts  encryption key, configuration file, defaults are used if NULL
+ */
 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 from 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
+ * @ret   EROFS if variable must not be changed (ethaddr, serial#)
+ */
 int fw_setenv(int argc, char *argv[], struct env_opts *opts);
+
+/**
+ * fw_parse_script - add/remove multiple variables with a batch script
+ *
+ * @fname  batch script file name
+ * @opts  encryption key, configuration file, defaults are used if NULL
+ *
+ * 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\n"
+ */
 int fw_parse_script(char *fname, struct env_opts *opts);
+
+
+/**
+ * fw_env_open - read enviroment from flash and cache it in RAM
+ *
+ * @opts  encryption key, configuration file, defaults are used if NULL
+ */
 int fw_env_open(struct env_opts *opts);
+
+/**
+ * fw_getenv - lookup variable in RAM cache
+ *
+ * @name  variable to be searched
+ * @ret   NULL if not found
+ */
+char *fw_getenv(char *name);
+
+/**
+ * fw_env_write - set/clear a single variable in the RAM cache
+ *
+ * This is called in sequence to update the environment in RAM without updating
+ * the copy in flash after each set
+ *
+ * @name  variable
+ * @value delete variable if NULL, otherwise create or overwrite the variable
+ */
 int fw_env_write(char *name, char *value);
+
+/**
+ * fw_env_close - writes environment from RAM back to flash
+ *
+ * @opts  encryption key, configuration file, defaults are used if NULL
+ */
 int fw_env_close(struct env_opts *opts);
 
 unsigned long crc32(unsigned long, const unsigned char *, unsigned);
-- 
2.8.1

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

[Andreas Fenkart]

From: Andreas Fenkart <afenkart@gmail.com>

forward declaration not needed when re-ordered

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 b1c8217..ca839d1 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 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 noheader.

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 ca839d1..9733950 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] [PATCH 1/4] tools/env: return with error if redundant environments have unequal size

[Simon Glass]

On 27 June 2016 at 16:06, Andreas Fenkart
<andreas.fenkart@digitalstrom.com> wrote:
>
> From: Andreas Fenkart <afenkart@gmail.com>
>
> 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.
>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
> ---
>  tools/env/fw_env.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

On 27 June 2016 at 16:06, Andreas Fenkart
<andreas.fenkart@digitalstrom.com> 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
>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
> ---
>  tools/env/fw_env.h | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 1 deletion(-)

Great to see this, thank you.

Reviewed-by: Simon Glass <sjg@chromium.org>

Nit below

>
> diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
> index dac964d..8bf74f3 100644
> --- a/tools/env/fw_env.h
> +++ b/tools/env/fw_env.h
> @@ -67,12 +67,85 @@ struct env_opts {
>
>  int parse_aes_key(char *key, uint8_t *bin_key);
>
> +/**
> + * fw_printenv - print one or several environment variables

We normally add function brackets:

   fw_printenv() - print ...

> + *
> + * @argc  number of variables names to be printed, print all if 0
> + * @argv  array of variable names to be printed
> + * @value_only  do not repeat the variable name, only print the value,
> + *          only one variable allowed with this option, argc must be 1
> + * @opts  encryption key, configuration file, defaults are used if NULL

@returns

> + */
>  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 from 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
> + * @ret   EROFS if variable must not be changed (ethaddr, serial#)

@returns

> + */
>  int fw_setenv(int argc, char *argv[], struct env_opts *opts);
> +
> +/**
> + * fw_parse_script - add/remove multiple variables with a batch script
> + *
> + * @fname  batch script file name
> + * @opts  encryption key, configuration file, defaults are used if NULL

@returns

(please do for all)

> + *
> + * 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\n"
> + */
>  int fw_parse_script(char *fname, struct env_opts *opts);
> +
> +
> +/**
> + * fw_env_open - read enviroment from flash and cache it in RAM
> + *
> + * @opts  encryption key, configuration file, defaults are used if NULL
> + */
>  int fw_env_open(struct env_opts *opts);
> +
> +/**
> + * fw_getenv - lookup variable in RAM cache
> + *
> + * @name  variable to be searched
> + * @ret   NULL if not found
> + */
> +char *fw_getenv(char *name);
> +
> +/**
> + * fw_env_write - set/clear a single variable in the RAM cache
> + *
> + * This is called in sequence to update the environment in RAM without updating
> + * the copy in flash after each set
> + *
> + * @name  variable
> + * @value delete variable if NULL, otherwise create or overwrite the variable
> + */
>  int fw_env_write(char *name, char *value);
> +
> +/**
> + * fw_env_close - writes environment from RAM back to flash
> + *
> + * @opts  encryption key, configuration file, defaults are used if NULL
> + */
>  int fw_env_close(struct env_opts *opts);
>
>  unsigned long crc32(unsigned long, const unsigned char *, unsigned);
> --
> 2.8.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

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

[Simon Glass]

On 27 June 2016 at 16:06, Andreas Fenkart
<andreas.fenkart@digitalstrom.com> wrote:
> From: Andreas Fenkart <afenkart@gmail.com>
>
> forward declaration not needed when re-ordered
>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
> ---
>  tools/env/fw_env.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Andreas Fenkart]

Hi Simon,

I sent out a v2 of this series, you should have received by now. Some
comments in the context:

2016-07-09 16:39 GMT+02:00 Simon Glass <sjg@chromium.org>:
> On 27 June 2016 at 16:06, Andreas Fenkart
> <andreas.fenkart@digitalstrom.com> wrote:

[snip]

>> +/**
>> + * fw_printenv - print one or several environment variables
>
> We normally add function brackets:

Hmm, I wonder what actually is used, since it's neither javadoc nor kernel-doc

http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html
http://www.mjmwired.net/kernel/Documentation/kernel-doc-nano-HOWTO.txt

For javadoc we would have to use @param <arg> keyword instead of the
shorter @arg used in the patch. But kernel-doc does not support
@return/@returns, it requires a 'Return:' section.

I verified the markup using:
./scripts/kernel-doc -html -v tools/env/fw_env.h

This was just for convenience, since the script comes with the source.
I can switch to whatever format is preferred.

>    fw_printenv() - print ...
>
>> + *
>> + * @argc  number of variables names to be printed, print all if 0
>> + * @argv  array of variable names to be printed
>> + * @value_only  do not repeat the variable name, only print the value,
>> + *          only one variable allowed with this option, argc must be 1
>> + * @opts  encryption key, configuration file, defaults are used if NULL
>
> @returns

$ grep -re  '\W at return\W' * | wc -l
1280

$ grep -re  '\W at returns\W' * | wc -l
12

javadoc uses @return

Is there a tool that processes the existing documentation markup,
probably it's not kernel-doc since it doesn't know the @return keyword

[snip]

/Andi

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

[Simon Glass]

+Marek

Hi Andreas,

On 16 July 2016 at 09:35, Andreas Fenkart <afenkart@gmail.com> wrote:
> Hi Simon,
>
> I sent out a v2 of this series, you should have received by now. Some
> comments in the context:
>
> 2016-07-09 16:39 GMT+02:00 Simon Glass <sjg@chromium.org>:
>> On 27 June 2016 at 16:06, Andreas Fenkart
>> <andreas.fenkart@digitalstrom.com> wrote:
>
> [snip]
>
>>> +/**
>>> + * fw_printenv - print one or several environment variables
>>
>> We normally add function brackets:
>
> Hmm, I wonder what actually is used, since it's neither javadoc nor kernel-doc
>
> http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html
> http://www.mjmwired.net/kernel/Documentation/kernel-doc-nano-HOWTO.txt
>
> For javadoc we would have to use @param <arg> keyword instead of the
> shorter @arg used in the patch. But kernel-doc does not support
> @return/@returns, it requires a 'Return:' section.
>
> I verified the markup using:
> ./scripts/kernel-doc -html -v tools/env/fw_env.h
>
> This was just for convenience, since the script comes with the source.
> I can switch to whatever format is preferred.
>
>>    fw_printenv() - print ...
>>
>>> + *
>>> + * @argc  number of variables names to be printed, print all if 0
>>> + * @argv  array of variable names to be printed
>>> + * @value_only  do not repeat the variable name, only print the value,
>>> + *          only one variable allowed with this option, argc must be 1
>>> + * @opts  encryption key, configuration file, defaults are used if NULL
>>
>> @returns
>
> $ grep -re  '\W at return\W' * | wc -l
> 1280
>
> $ grep -re  '\W at returns\W' * | wc -l
> 12
>
> javadoc uses @return
>
> Is there a tool that processes the existing documentation markup,
> probably it's not kernel-doc since it doesn't know the @return keyword

I'm (hopefully) following instructions from Marek who may have more
info on this. I believe it is docbook (make pdfdocs), but only a few
files are included at present.

Regards,
Simon

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

[Marek Vasut]

On 07/17/2016 04:12 PM, Simon Glass wrote:
> +Marek
>
> Hi Andreas,
>
> On 16 July 2016 at 09:35, Andreas Fenkart <afenkart@gmail.com> wrote:
>> Hi Simon,
>>
>> I sent out a v2 of this series, you should have received by now. Some
>> comments in the context:
>>
>> 2016-07-09 16:39 GMT+02:00 Simon Glass <sjg@chromium.org>:
>>> On 27 June 2016 at 16:06, Andreas Fenkart
>>> <andreas.fenkart@digitalstrom.com> wrote:
>>
>> [snip]
>>
>>>> +/**
>>>> + * fw_printenv - print one or several environment variables
>>>
>>> We normally add function brackets:
>>
>> Hmm, I wonder what actually is used, since it's neither javadoc nor kernel-doc
>>
>> http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html
>> http://www.mjmwired.net/kernel/Documentation/kernel-doc-nano-HOWTO.txt
>>
>> For javadoc we would have to use @param <arg> keyword instead of the
>> shorter @arg used in the patch. But kernel-doc does not support
>> @return/@returns, it requires a 'Return:' section.
>>
>> I verified the markup using:
>> ./scripts/kernel-doc -html -v tools/env/fw_env.h
>>
>> This was just for convenience, since the script comes with the source.
>> I can switch to whatever format is preferred.
>>
>>>    fw_printenv() - print ...
>>>
>>>> + *
>>>> + * @argc  number of variables names to be printed, print all if 0
>>>> + * @argv  array of variable names to be printed
>>>> + * @value_only  do not repeat the variable name, only print the value,
>>>> + *          only one variable allowed with this option, argc must be 1
>>>> + * @opts  encryption key, configuration file, defaults are used if NULL
>>>
>>> @returns
>>
>> $ grep -re  '\W at return\W' * | wc -l
>> 1280
>>
>> $ grep -re  '\W at returns\W' * | wc -l
>> 12
>>
>> javadoc uses @return
>>
>> Is there a tool that processes the existing documentation markup,
>> probably it's not kernel-doc since it doesn't know the @return keyword
>
> I'm (hopefully) following instructions from Marek who may have more
> info on this. I believe it is docbook (make pdfdocs), but only a few
> files are included at present.

I'm not sure what the question is, I didn't manage to extrapolate it 
from the previous discussion -- but kerneldoc is the preferred doc 
format for u-boot.


-- 
Best regards,
Marek Vasut