[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility

[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility

[Stefano Babic]

Add a sort of batch mode to fw_setenv, allowing to set
multiple variables in one shot, without updating the flash after
each set as now. It is added the possibility to pass
a config file with a list of pairs <variable, value> to be set,
separated by a TAB character.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 tools/env/fw_env.c      |  216 +++++++++++++++++++++++++++++++++++++++--------
 tools/env/fw_env.h      |    7 ++
 tools/env/fw_env_main.c |   32 +++++++-
 3 files changed, 218 insertions(+), 37 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index a46205d..506a806 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -45,9 +45,6 @@
 
 #include "fw_env.h"
 
-#define	CMD_GETENV	"fw_printenv"
-#define	CMD_SETENV	"fw_setenv"
-
 #define min(x, y) ({				\
 	typeof(x) _min1 = (x);			\
 	typeof(y) _min2 = (y);			\
@@ -328,29 +325,15 @@ int fw_printenv (int argc, char *argv[])
 }
 
 /*
- * Deletes or sets environment variables. Returns -1 and sets errno error codes:
- * 0	  - OK
- * EINVAL - need at least 1 argument
- * EROFS  - certain variables ("ethaddr", "serial#") cannot be
- *	    modified or deleted
- *
+ * Set/Clear a single variable in the environment.
+ * This is called in sequence to update the environment
+ * in RAM without updating the copy in flash after each set
  */
-int fw_setenv (int argc, char *argv[])
+int fw_set_single_var(char *name, char *value)
 {
-	int i, len;
+	int len;
 	char *env, *nxt;
 	char *oldval = NULL;
-	char *name;
-
-	if (argc < 2) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	if (env_init ())
-		return -1;
-
-	name = argv[1];
 
 	/*
 	 * search if variable with this name already exists
@@ -358,7 +341,7 @@ int fw_setenv (int argc, char *argv[])
 	for (nxt = env = environment.data; *env; env = nxt + 1) {
 		for (nxt = env; *nxt; ++nxt) {
 			if (nxt >= &environment.data[ENV_SIZE]) {
-				fprintf (stderr, "## Error: "
+				fprintf(stderr, "## Error: "
 					"environment not terminated\n");
 				errno = EINVAL;
 				return -1;
@@ -396,8 +379,8 @@ int fw_setenv (int argc, char *argv[])
 	}
 
 	/* Delete only ? */
-	if (argc < 3)
-		goto WRITE_FLASH;
+	if (!value || !strlen(value))
+		return 0;
 
 	/*
 	 * Append new definition@the end
@@ -411,41 +394,204 @@ int fw_setenv (int argc, char *argv[])
 	 */
 	len = strlen (name) + 2;
 	/* add '=' for first arg, ' ' for all others */
-	for (i = 2; i < argc; ++i) {
-		len += strlen (argv[i]) + 1;
-	}
+	len += strlen(value) + 1;
+
 	if (len > (&environment.data[ENV_SIZE] - env)) {
 		fprintf (stderr,
 			"Error: environment overflow, \"%s\" deleted\n",
 			name);
 		return -1;
 	}
+
 	while ((*env = *name++) != '\0')
 		env++;
+	*env = '=';
+	while ((*++env = *value++) != '\0')
+		;
+
+	/* end is marked with double '\0' */
+	*++env = '\0';
+
+	return 0;
+}
+
+/*
+ * Deletes or sets environment variables. Returns -1 and sets errno error codes:
+ * 0	  - OK
+ * EINVAL - need at least 1 argument
+ * EROFS  - certain variables ("ethaddr", "serial#") cannot be
+ *	    modified or deleted
+ *
+ */
+int fw_setenv(int argc, char *argv[])
+{
+	int i, len;
+	char *env;
+	char *name;
+	char *value = NULL;
+	char *tmpval = NULL;
+
+	if (argc < 2) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (env_init())
+		return -1;
+
+	name = argv[1];
+
+	/*
+	 * Overflow when:
+	 * "name" + "=" + "val" +"\0\0"  > CONFIG_ENV_SIZE - (env-environment)
+	 */
+	len = strlen(name) + 2;
+	/* add '=' for first arg, ' ' for all others */
+	for (i = 2; i < argc; ++i)
+		len += strlen(argv[i]) + 1;
+
+	if (len > (&environment.data[ENV_SIZE] - env)) {
+		fprintf(stderr,
+			"Error: environment overflow, \"%s\" deleted\n",
+			name);
+		return -1;
+	}
+
+	/* Allocate enough place to the data string */
 	for (i = 2; i < argc; ++i) {
 		char *val = argv[i];
+		if (!value) {
+			value = (char *)malloc(len - strlen(name));
+			memset(value, 0, len - strlen(name));
+			tmpval = value;
+		}
+		if (i != 2)
+			*tmpval++ = ' ';
+		while (*val != '\0')
+			*tmpval++ = *val++;
+	}
+
+	fw_set_single_var(name, value);
 
-		*env = (i == 2) ? '=' : ' ';
-		while ((*++env = *val++) != '\0');
+	/*
+	 * Update CRC
+	 */
+	*environment.crc = crc32(0, (uint8_t *) environment.data, ENV_SIZE);
+
+	/* write environment back to flash */
+	if (flash_io(O_RDWR)) {
+		fprintf(stderr, "Error: can't write fw_env to flash\n");
+		return -1;
 	}
 
-	/* end is marked with double '\0' */
-	*++env = '\0';
+	if (value)
+		free(value);
+
+	return 0;
 
-  WRITE_FLASH:
+}
+
+/*
+ * Deletes or sets multiple environment variables.
+ * Returns -1 and sets errno error codes:
+ * 0	  - OK
+ * EINVAL - need@least 1 argument
+ * EROFS  - certain variables ("ethaddr", "serial#") cannot be
+ *	    modified or deleted
+ *
+ */
+int fw_setenv_multiple(fw_env_list *list, int count)
+{
+
+	int i, ret;
+
+	if (env_init())
+		return -1;
+
+	for (i = 0; i < count; i++) {
+		ret = fw_set_single_var(list[i].name, list[i].value);
+		if (ret) {
+			fprintf(stderr, "Error: can't set %s to %s\n",
+				list[i].name, list[i].value);
+			return -1;
+		}
+	}
 
 	/*
 	 * Update CRC
 	 */
-	*environment.crc = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
+	*environment.crc = crc32(0, (uint8_t *) environment.data, ENV_SIZE);
 
 	/* write environment back to flash */
-	if (flash_io (O_RDWR)) {
+	if (flash_io(O_RDWR)) {
 		fprintf (stderr, "Error: can't write fw_env to flash\n");
 		return -1;
 	}
 
 	return 0;
+
+}
+
+/*
+ * Parse script to generate list of variables to set
+ * The script file has a very simple format, as follows:
+ *
+ * Each line has a couple with name, value:
+ * variable_name<TAB>variable_value
+ *
+ * Both variable_name and variable_value are interpreted as strings.
+ * Any character after <TAB> and before ending \r\n is interpreted
+ * as variable's value (no comment allowed on these lines !)
+ *
+ * Comments are allowed if the first character in the line is #
+ *
+ * Returns -1 and sets errno error codes:
+ * 0	  - OK
+ * -1     - Error
+ */
+int fw_parse_script(char *fname, fw_env_list *list, int count)
+{
+	FILE *fp;
+	int i = 0;
+	char dump[128];
+	char *name;
+	char *val;
+
+
+	fp = fopen(fname, "r");
+	if (fp == NULL)
+		return -1;
+
+	while (fgets(dump, sizeof(dump), fp)) {
+		/* Skip incomplete conversions and comment strings */
+		if (dump[0] == '#')
+			continue;
+
+		list[i].name[0] = '\0';
+		list[i].value[0] = '\0';
+
+		val = strtok(dump, "\r\n");
+		if (!val)
+			continue;
+
+		name = strtok(dump, "\t");
+		if (!name)
+			continue;
+
+		strncpy(list[i].name, name, sizeof(list[i].name) - 1);
+
+		val = strtok(NULL, "\t");
+		if (val)
+			strncpy(list[i].value, val, sizeof(list[i].value) - 1);
+
+		printf("name = %s value = %s\n", list[i].name, list[i].value);
+		i++;
+
+		if (i >= count)
+			return count;
+	}
+
+	return i;
 }
 
 /*
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
index c04da54..bd494c0 100644
--- a/tools/env/fw_env.h
+++ b/tools/env/fw_env.h
@@ -47,8 +47,15 @@
 	"ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; "	\
 	"bootm"
 
+typedef struct {
+	char name[255];
+	char value[255];
+} fw_env_list;
+
 extern int   fw_printenv(int argc, char *argv[]);
 extern char *fw_getenv  (char *name);
 extern int fw_setenv  (int argc, char *argv[]);
+extern int fw_parse_script(char *fname, fw_env_list *list, int count);
+extern int fw_setenv_multiple(fw_env_list *list, int count);
 
 extern unsigned	long  crc32	 (unsigned long, const unsigned char *, unsigned);
diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c
index 7f631c4..42a7168 100644
--- a/tools/env/fw_env_main.c
+++ b/tools/env/fw_env_main.c
@@ -42,16 +42,26 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
+#include <getopt.h>
 #include "fw_env.h"
 
 #define	CMD_PRINTENV	"fw_printenv"
 #define CMD_SETENV	"fw_setenv"
+#define MAX_SET_VARS	1024
+
+static struct option long_options[] = {
+	{"script", required_argument, NULL, 's'},
+	{NULL, 0, NULL, 0}
+};
 
 int
 main(int argc, char *argv[])
 {
 	char *p;
 	char *cmdname = *argv;
+	char *script_file = NULL;
+	fw_env_list *list;
+	int c, count;
 
 	if ((p = strrchr (cmdname, '/')) != NULL) {
 		cmdname = p + 1;
@@ -65,9 +75,27 @@ main(int argc, char *argv[])
 		return (EXIT_SUCCESS);
 
 	} else if (strcmp(cmdname, CMD_SETENV) == 0) {
+		while ((c = getopt_long (argc, argv, "s:",
+				long_options, NULL)) != EOF) {
+			switch (c) {
+			case 's':
+				script_file = optarg;
+				break;
+			}
+		}
 
-		if (fw_setenv (argc, argv) != 0)
-			return (EXIT_FAILURE);
+		if (!script_file) {
+			if (fw_setenv(argc, argv) != 0)
+				return EXIT_FAILURE;
+		} else {
+			list = (fw_env_list *)malloc(MAX_SET_VARS *
+					sizeof(fw_env_list));
+			count = fw_parse_script(script_file,
+					list, MAX_SET_VARS);
+
+			if (fw_setenv_multiple(list, count) != 0)
+				return EXIT_FAILURE;
+		}
 
 		return (EXIT_SUCCESS);
 
-- 
1.6.3.3

[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility

[Wolfgang Denk]

Dear Stefano Babic,

In message <1274439131-22807-1-git-send-email-sbabic@denx.de> you wrote:
> Add a sort of batch mode to fw_setenv, allowing to set
> multiple variables in one shot, without updating the flash after
> each set as now. It is added the possibility to pass

Thanks; that's a welcome improvement!

> a config file with a list of pairs <variable, value> to be set,
> separated by a TAB character.

I think we should be less restrictive here. Please split at the first
white space, and allow for multiple white spaces.


> +	/* Allocate enough place to the data string */
>  	for (i = 2; i < argc; ++i) {
>  		char *val = argv[i];
> +		if (!value) {
> +			value = (char *)malloc(len - strlen(name));
> +			memset(value, 0, len - strlen(name));

Kaboom! when malloc() fails.

> +			tmpval = value;
> +		}
> +		if (i != 2)
> +			*tmpval++ = ' ';
> +		while (*val != '\0')
> +			*tmpval++ = *val++;
> +	}
> +
> +	fw_set_single_var(name, value);

Silent corruption of environment when malloc() failed.

> +	/*
> +	 * Update CRC
> +	 */
> +	*environment.crc = crc32(0, (uint8_t *) environment.data, ENV_SIZE);
> +
> +	/* write environment back to flash */
> +	if (flash_io(O_RDWR)) {
> +		fprintf(stderr, "Error: can't write fw_env to flash\n");
> +		return -1;
...
>  	/*
>  	 * Update CRC
>  	 */
> -	*environment.crc = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
> +	*environment.crc = crc32(0, (uint8_t *) environment.data, ENV_SIZE);
>  
>  	/* write environment back to flash */
> -	if (flash_io (O_RDWR)) {
> +	if (flash_io(O_RDWR)) {
>  		fprintf (stderr, "Error: can't write fw_env to flash\n");
>  		return -1;

We probably should not repeat this code, but factor it out.

> +/*
> + * Parse script to generate list of variables to set
> + * The script file has a very simple format, as follows:
> + *
> + * Each line has a couple with name, value:
> + * variable_name<TAB>variable_value

Please make this:

	name<white space>value

> + * Both variable_name and variable_value are interpreted as strings.
> + * Any character after <TAB> and before ending \r\n is interpreted
> + * as variable's value (no comment allowed on these lines !)
> + *
> + * Comments are allowed if the first character in the line is #
> + *
> + * Returns -1 and sets errno error codes:
> + * 0	  - OK
> + * -1     - Error
> + */
> +int fw_parse_script(char *fname, fw_env_list *list, int count)
> +{
> +	FILE *fp;
> +	int i = 0;
> +	char dump[128];

Ouch! That's short! Why do we need such an arbitrary limit?

> +	fp = fopen(fname, "r");
> +	if (fp == NULL)
> +		return -1;

How about a useful error message?

> +	while (fgets(dump, sizeof(dump), fp)) {
> +		/* Skip incomplete conversions and comment strings */
> +		if (dump[0] == '#')
> +			continue;

What are incomplete conversions, and where do they get skipped? And
what happens with the rest of the input?

> +		list[i].name[0] = '\0';
> +		list[i].value[0] = '\0';
> +
> +		val = strtok(dump, "\r\n");
> +		if (!val)
> +			continue;
> +
> +		name = strtok(dump, "\t");
> +		if (!name)
> +			continue;
> +
> +		strncpy(list[i].name, name, sizeof(list[i].name) - 1);

Error message for too long names?

> +		val = strtok(NULL, "\t");
> +		if (val)
> +			strncpy(list[i].value, val, sizeof(list[i].value) - 1);

Error message for too long values?


Hm... Isn't strtok() a bit of overkill here, when all we want to do is
split at the first white space?

> +typedef struct {
> +	char name[255];
> +	char value[255];
> +} fw_env_list;

Again we have arbitrary limits. And even different ones from the one
above (128).


> +static struct option long_options[] = {
> +	{"script", required_argument, NULL, 's'},
> +	{NULL, 0, NULL, 0}
> +};

The command should also accept '-' as file name, and then read from
stdin.

> +		if (!script_file) {
> +			if (fw_setenv(argc, argv) != 0)
> +				return EXIT_FAILURE;
> +		} else {
> +			list = (fw_env_list *)malloc(MAX_SET_VARS *
> +					sizeof(fw_env_list));
> +			count = fw_parse_script(script_file,
> +					list, MAX_SET_VARS);

Kaboom! when malloc() failed.




Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"In matrimony, to hesitate is sometimes to be saved."        - Butler

[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility

[Stefano Babic]

Wolfgang Denk wrote:

>> a config file with a list of pairs <variable, value> to be set,
>> separated by a TAB character.
> 
> I think we should be less restrictive here. Please split at the first
> white space, and allow for multiple white spaces.

There is a reason why I set to this format. There should be a case where
a variable is set with a leading (or more as one) white space. For
example, I have seen something related to board names, and adding some
spaces makes a sort of formatting without changing the code.

Because the TAB character is not allowed inside a variable, this assures
that everything except TAB is taken as part of the variables's value.

>> +	/* Allocate enough place to the data string */
>>  	for (i = 2; i < argc; ++i) {
>>  		char *val = argv[i];
>> +		if (!value) {
>> +			value = (char *)malloc(len - strlen(name));
>> +			memset(value, 0, len - strlen(name));
> 
> Kaboom! when malloc() fails.

This is an error, thanks. And I see I have not checked the return value
of malloc at all in my patch. I will fix it globally.

>>  	/* write environment back to flash */
>> -	if (flash_io (O_RDWR)) {
>> +	if (flash_io(O_RDWR)) {
>>  		fprintf (stderr, "Error: can't write fw_env to flash\n");
>>  		return -1;
> 
> We probably should not repeat this code, but factor it out.

Good idea, sure !

> 
>> +/*
>> + * Parse script to generate list of variables to set
>> + * The script file has a very simple format, as follows:
>> + *
>> + * Each line has a couple with name, value:
>> + * variable_name<TAB>variable_value
> 
> Please make this:
> 
> 	name<white space>value

See my comment above. With my proposal, something as

board_name<TAB>    my product made by my company

works flawlessy. We can add some delimiters (as ', "), but we could have
such delimiters inside the value itself and we have then to use escapes.
Because <TAB> is not allowed as character inside a variable (or I have
not yet seen this case...), it makes thing easier.

>> +	int i = 0;
>> +	char dump[128];
> 
> Ouch! That's short! Why do we need such an arbitrary limit?

We do not need a small value, but I have to set a value for fgets. A
bigger value should be enough, reporting an error if the line is too long.

> 
>> +	fp = fopen(fname, "r");
>> +	if (fp == NULL)
>> +		return -1;
> 
> How about a useful error message?

Surely.

>> +		strncpy(list[i].name, name, sizeof(list[i].name) - 1);
> 
> Error message for too long names?

Yes !

> Hm... Isn't strtok() a bit of overkill here, when all we want to do is
> split at the first white space?

Well, I have not thought it is a problem. Of course the simple way could
be to check the whole bytes inside the string, but why do you think I
should not use strtok ?

> 
>> +typedef struct {
>> +	char name[255];
>> +	char value[255];
>> +} fw_env_list;
> 
> Again we have arbitrary limits. And even different ones from the one
> above (128).

Another possibility is to dinamically allocate memory for name/value on
demand. Theoretically a value could be a very long string and the only
limit I see in u-boot is the environment size.

> 
> 
>> +static struct option long_options[] = {
>> +	{"script", required_argument, NULL, 's'},
>> +	{NULL, 0, NULL, 0}
>> +};
> 
> The command should also accept '-' as file name, and then read from
> stdin.

Ok, understood. So we can allow something like "fw_setenv -s - <
variables_file".

Regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
=====================================================================

[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility

[Wolfgang Denk]

Dear Stefano Babic,

In message <4BF6A381.7020200@denx.de> you wrote:
> Wolfgang Denk wrote:
> 
> >> a config file with a list of pairs <variable, value> to be set,
> >> separated by a TAB character.
> > 
> > I think we should be less restrictive here. Please split at the first
> > white space, and allow for multiple white spaces.
> 
> There is a reason why I set to this format. There should be a case where
> a variable is set with a leading (or more as one) white space. For
> example, I have seen something related to board names, and adding some
> spaces makes a sort of formatting without changing the code.

Adding spaces to the variable value? This makes little sense to me.
It's just a waste of storage space and boot time.

If you feel your environment is so complicated to read that you want
such "formatting", then rather structure your envrionment (see
proposals on the list), and/or help improving the printenv
capabilities to add sorting etc.

> Because the TAB character is not allowed inside a variable, this assures
> that everything except TAB is taken as part of the variables's value.

TAB can be embedded in a variable value (if you manage to enter it).

> See my comment above. With my proposal, something as
> 
> board_name<TAB>    my product made by my company

> works flawlessy. We can add some delimiters (as ', "), but we could have
> such delimiters inside the value itself and we have then to use escapes.
> Because <TAB> is not allowed as character inside a variable (or I have
> not yet seen this case...), it makes thing easier.

Who says that TAB would not be allowed? Of course it is. There is
virtually no restriction on the content of the value of a variable.
The only character that cannot be part of the value is '\0'.

[Of course it is not a wise decision to add non-printing or control
characters, but you can do this, if you like.]

> >> +	char dump[128];
> > 
> > Ouch! That's short! Why do we need such an arbitrary limit?
> 
> We do not need a small value, but I have to set a value for fgets. A
> bigger value should be enough, reporting an error if the line is too long.

But your code is not set up to handle the remainder of too long lines.
It would be read as the next line and cause confusion.

> >> +typedef struct {
> >> +	char name[255];
> >> +	char value[255];
> >> +} fw_env_list;
> >
> > Again we have arbitrary limits. And even different ones from the one
> > above (128).
>
> Another possibility is to dinamically allocate memory for name/value on
> demand. Theoretically a value could be a very long string and the only
> limit I see in u-boot is the environment size.

Why do you have to run this in two spearate passes at all - first
scan, then process.

Why cannot you process each variable when you read it? The we would
not need any array or list at all.

> > The command should also accept '-' as file name, and then read from
> > stdin.
>
> Ok, understood. So we can allow something like "fw_setenv -s - <
> variables_file".

Right.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I must follow the people.  Am I not their leader? - Benjamin Disraeli

[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility

[Stefano Babic]

Wolfgang Denk wrote:

> Adding spaces to the variable value? This makes little sense to me.
> It's just a waste of storage space and boot time.

Well, I can agree with you, however I have already seen this case...

> If you feel your environment is so complicated to read that you want
> such "formatting", then rather structure your envrionment (see
> proposals on the list), and/or help improving the printenv
> capabilities to add sorting etc.

Ok, understood. I will change to support something like

<whitespaces>variable<whitespaces>value

> Who says that TAB would not be allowed?

..probably is more stranger as to have spaces....

> Of course it is. There is
> virtually no restriction on the content of the value of a variable.
> The only character that cannot be part of the value is '\0'.
> 
> [Of course it is not a wise decision to add non-printing or control
> characters, but you can do this, if you like.]

As I saw some leading whitespaces in a variable, I cannot remember a
case where I saw a TAB in a variable. But as it is not forbidden,
probably there is still this case !

> 
>>>> +	char dump[128];
>>> Ouch! That's short! Why do we need such an arbitrary limit?
>> We do not need a small value, but I have to set a value for fgets. A
>> bigger value should be enough, reporting an error if the line is too long.
> 
> But your code is not set up to handle the remainder of too long lines.
> It would be read as the next line and cause confusion.

Ah, ok, I get now the point.

> Why do you have to run this in two spearate passes at all - first
> scan, then process.
> 
> Why cannot you process each variable when you read it? The we would
> not need any array or list at all.

There is a reason, please tell me how good it is ;-)

I thought the code could be used not only as it is, but integrated in an
application linking the required object (only one, fw_env.o).
An application could internally generate a list of pairs variable/values
and needs a function to set them in the u-boot environment. This is the
reason of the int fw_setenv_multiple(fw_env_list *list, int count)
function. No need to use an external file, no need to call fw_setenv as
external process. An application could link fw_env.o and call
fw_setenv_multiple().

I agree with you that I can do everything in a single pass if we do not
provide such kind of external interface.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility

[Wolfgang Denk]

Dear Stefano,

In message <4BF6B724.6090307@denx.de> you wrote:
> 
> > Adding spaces to the variable value? This makes little sense to me.
> > It's just a waste of storage space and boot time.
> 
> Well, I can agree with you, however I have already seen this case...

I believe you. I have seen stranger things before ;-)

> > Why cannot you process each variable when you read it? The we would
> > not need any array or list at all.
> 
> There is a reason, please tell me how good it is ;-)
> 
> I thought the code could be used not only as it is, but integrated in an
> application linking the required object (only one, fw_env.o).
> An application could internally generate a list of pairs variable/values
> and needs a function to set them in the u-boot environment. This is the
> reason of the int fw_setenv_multiple(fw_env_list *list, int count)
> function. No need to use an external file, no need to call fw_setenv as
> external process. An application could link fw_env.o and call
> fw_setenv_multiple().
> 
> I agree with you that I can do everything in a single pass if we do not
> provide such kind of external interface.

Even then you probably can. Actually it may be easier for the
aplication, too, when you provide something similar to classic file
I/O interface - fw_env_open() would initialize the in-memory copy of
the environment data, fw_env_write() could be called repeatedly to
set or delete variables, one at a time, and fw_env_close() would
finally compute the CRC and write the data out to persistent storage.

I guess in most cases such an interface is easier for the appli-
cation, too, because it can handle variables as they appear, and it
does not need to provide yet another array (with all the size
limitations) for intermediate storage.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"One planet is all you get."

[U-Boot] [PATCH V2] Tools: set multiple variable with fw_setenv utility

[Stefano Babic]

Add a sort of batch mode to fw_setenv, allowing to set
multiple variables in one shot, without updating the flash after
each set as now. It is added the possibility to pass
a config file with a list of pairs <variable, value> to be set,
separated by a TAB character.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 tools/env/fw_env.c      |  279 ++++++++++++++++++++++++++++++++++++++++-------
 tools/env/fw_env.h      |    1 +
 tools/env/fw_env_main.c |   25 ++++-
 3 files changed, 263 insertions(+), 42 deletions(-)

Changes since V1:

Wolfgang Denk:
	- check returns of malloc call (no malloc required in this version)
	- added messages in case of errors
	- replace interface with a more similar I/O interface (fw_env_open,
	fw_env_write, fw_env_close)
	- Allow any number of leading whitespaces in the file format
	<white spaces>name<white space>value
	- Refactoring out some general code
	- Check for too long lines
	- Accept stdin as filename


diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index a46205d..6bf2761 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -45,8 +45,7 @@
 
 #include "fw_env.h"
 
-#define	CMD_GETENV	"fw_printenv"
-#define	CMD_SETENV	"fw_setenv"
+#define WHITESPACE(c) ((c == '\t') || (c == ' '))
 
 #define min(x, y) ({				\
 	typeof(x) _min1 = (x);			\
@@ -225,6 +224,22 @@ static inline ulong getenvsize (void)
 	return rc;
 }
 
+static char *fw_string_blank(char *s, int noblank)
+{
+	int i;
+	int len = strlen(s);
+
+	for (i = 0; i < len; i++, s++) {
+		if ((noblank && !WHITESPACE(*s)) ||
+			(!noblank && WHITESPACE(*s)))
+			break;
+	}
+	if (i == len)
+		return NULL;
+
+	return s;
+}
+
 /*
  * Search the environment for a variable.
  * Return the value, if found, or NULL, if not found.
@@ -327,30 +342,39 @@ int fw_printenv (int argc, char *argv[])
 	return rc;
 }
 
-/*
- * Deletes or sets environment variables. Returns -1 and sets errno error codes:
- * 0	  - OK
- * EINVAL - need@least 1 argument
- * EROFS  - certain variables ("ethaddr", "serial#") cannot be
- *	    modified or deleted
- *
- */
-int fw_setenv (int argc, char *argv[])
+int fw_env_open(void)
 {
-	int i, len;
-	char *env, *nxt;
-	char *oldval = NULL;
-	char *name;
+	return env_init();
+}
 
-	if (argc < 2) {
-		errno = EINVAL;
-		return -1;
+int fw_env_close(void)
+{
+	/*
+	 * Update CRC
+	 */
+	*environment.crc = crc32(0, (uint8_t *) environment.data, ENV_SIZE);
+
+	/* write environment back to flash */
+	if (flash_io(O_RDWR)) {
+		fprintf(stderr,
+			"Error: can't write fw_env to flash\n");
+			return -1;
 	}
 
-	if (env_init ())
-		return -1;
+	return 0;
+}
 
-	name = argv[1];
+
+/*
+ * Set/Clear a single variable in the environment.
+ * This is called in sequence to update the environment
+ * in RAM without updating the copy in flash after each set
+ */
+int fw_env_write(char *name, char *value)
+{
+	int len;
+	char *env, *nxt;
+	char *oldval = NULL;
 
 	/*
 	 * search if variable with this name already exists
@@ -358,7 +382,7 @@ int fw_setenv (int argc, char *argv[])
 	for (nxt = env = environment.data; *env; env = nxt + 1) {
 		for (nxt = env; *nxt; ++nxt) {
 			if (nxt >= &environment.data[ENV_SIZE]) {
-				fprintf (stderr, "## Error: "
+				fprintf(stderr, "## Error: "
 					"environment not terminated\n");
 				errno = EINVAL;
 				return -1;
@@ -396,8 +420,8 @@ int fw_setenv (int argc, char *argv[])
 	}
 
 	/* Delete only ? */
-	if (argc < 3)
-		goto WRITE_FLASH;
+	if (!value || !strlen(value))
+		return 0;
 
 	/*
 	 * Append new definition@the end
@@ -411,41 +435,216 @@ int fw_setenv (int argc, char *argv[])
 	 */
 	len = strlen (name) + 2;
 	/* add '=' for first arg, ' ' for all others */
-	for (i = 2; i < argc; ++i) {
-		len += strlen (argv[i]) + 1;
-	}
+	len += strlen(value) + 1;
+
 	if (len > (&environment.data[ENV_SIZE] - env)) {
 		fprintf (stderr,
 			"Error: environment overflow, \"%s\" deleted\n",
 			name);
 		return -1;
 	}
+
 	while ((*env = *name++) != '\0')
 		env++;
-	for (i = 2; i < argc; ++i) {
-		char *val = argv[i];
-
-		*env = (i == 2) ? '=' : ' ';
-		while ((*++env = *val++) != '\0');
-	}
+	*env = '=';
+	while ((*++env = *value++) != '\0')
+		;
 
 	/* end is marked with double '\0' */
 	*++env = '\0';
 
-  WRITE_FLASH:
+	return 0;
+}
+
+/*
+ * Deletes or sets environment variables. Returns -1 and sets errno error codes:
+ * 0	  - OK
+ * EINVAL - need@least 1 argument
+ * EROFS  - certain variables ("ethaddr", "serial#") cannot be
+ *	    modified or deleted
+ *
+ */
+int fw_setenv(int argc, char *argv[])
+{
+	int i, len;
+	char *env;
+	char *name;
+	char *value = NULL;
+	char *tmpval = NULL;
+
+	if (argc < 2) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (fw_env_open()) {
+		fprintf(stderr, "Error: environment not initialized\n");
+		return -1;
+	}
+
+	name = argv[1];
 
 	/*
-	 * Update CRC
+	 * Overflow when:
+	 * "name" + "=" + "val" +"\0\0"  > CONFIG_ENV_SIZE - (env-environment)
 	 */
-	*environment.crc = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
+	len = strlen(name) + 2;
+	/* add '=' for first arg, ' ' for all others */
+	for (i = 2; i < argc; ++i)
+		len += strlen(argv[i]) + 1;
 
-	/* write environment back to flash */
-	if (flash_io (O_RDWR)) {
-		fprintf (stderr, "Error: can't write fw_env to flash\n");
+	if (len > (&environment.data[ENV_SIZE] - env)) {
+		fprintf(stderr,
+			"Error: environment overflow, \"%s\" deleted\n",
+			name);
 		return -1;
 	}
 
-	return 0;
+	/* Allocate enough place to the data string */
+	for (i = 2; i < argc; ++i) {
+		char *val = argv[i];
+		if (!value) {
+			value = (char *)malloc(len - strlen(name));
+			if (!value) {
+				fprintf(stderr,
+				"Cannot malloc %u bytes: %s\n",
+				len - strlen(name), strerror(errno));
+				return -1;
+			}
+			memset(value, 0, len - strlen(name));
+			tmpval = value;
+		}
+		if (i != 2)
+			*tmpval++ = ' ';
+		while (*val != '\0')
+			*tmpval++ = *val++;
+	}
+
+	fw_env_write(name, value);
+
+	if (value)
+		free(value);
+
+	return fw_env_close();
+}
+
+/*
+ * Parse  a file  and configure the u-boot variables.
+ * The script file has a very simple format, as follows:
+ *
+ * Each line has a couple with name, value:
+ * <white spaces>variable_name<white spaces>variable_value
+ *
+ * Both variable_name and variable_value are interpreted as strings.
+ * Any character after <white spaces> and before ending \r\n is interpreted
+ * as variable's value (no comment allowed on these lines !)
+ *
+ * Comments are allowed if the first character in the line is #
+ *
+ * Returns -1 and sets errno error codes:
+ * 0	  - OK
+ * -1     - Error
+ */
+int fw_parse_script(char *fname)
+{
+	FILE *fp;
+	char dump[1024];	/* Maximum line length in the file */
+	char *name;
+	char *val;
+	int lineno = 0;
+	int len;
+	int ret = 0;
+
+	if (fw_env_open()) {
+		fprintf(stderr, "Error: environment not initialized\n");
+		return -1;
+	}
+
+	if (strcmp(fname, "-") == 0)
+		fp = stdin;
+	else {
+		fp = fopen(fname, "r");
+		if (fp == NULL) {
+			fprintf(stderr, "I cannot open %s for reading\n",
+				 fname);
+			return -1;
+		}
+	}
+
+	while (fgets(dump, sizeof(dump), fp)) {
+		lineno++;
+		len = strlen(dump);
+
+		/*
+		 * Read a whole line from the file. If the line is too long
+		 * or is not terminated, reports an error and exit.
+		 */
+		if (dump[len - 1] != '\n') {
+			fprintf(stderr,
+			"Line %d not corrected terminated or too long\n",
+				lineno);
+			ret = -1;
+			break;
+		}
+
+		/* Drop ending line feed / carriage return */
+		while (len > 0 && (dump[len - 1] == '\n' ||
+				dump[len - 1] == '\r')) {
+			dump[len - 1] = '\0';
+			len--;
+		}
+
+		/* Skip comment or empty lines */
+		if ((len == 0) || dump[0] == '#')
+			continue;
+
+		/*
+		 * Search for variable's name,
+		 * remove leading whitespaces
+		 */
+		name = fw_string_blank(dump, 1);
+		if (!name)
+			continue;
+
+		/* The first white space is the end of variable name */
+		val = fw_string_blank(name, 0);
+		len = strlen(name);
+		if (val) {
+			*val++ = '\0';
+			if ((val - name) < len)
+				val = fw_string_blank(val, 1);
+			else
+				val = NULL;
+		}
+
+#ifdef DEBUG
+		fprintf(stderr, "Setting %s : %s\n",
+			name, val ? val : " removed");
+#endif
+
+		/*
+		 * If there is an error setting a variable,
+		 * try to save the environment and returns an error
+		 */
+		if (fw_env_write(name, val)) {
+			fprintf(stderr,
+			"fw_env_write returns with error : %s\n",
+				strerror(errno));
+			fw_env_close();
+			ret = -1;
+			break;
+		}
+
+	}
+
+	/* Close file if not stdin */
+	if (strcmp(fname, "-") != 0)
+		fclose(fp);
+
+	ret |= fw_env_close();
+
+	return ret;
+
 }
 
 /*
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
index c04da54..f24d7a9 100644
--- a/tools/env/fw_env.h
+++ b/tools/env/fw_env.h
@@ -50,5 +50,6 @@
 extern int   fw_printenv(int argc, char *argv[]);
 extern char *fw_getenv  (char *name);
 extern int fw_setenv  (int argc, char *argv[]);
+extern int fw_parse_script(char *fname);
 
 extern unsigned	long  crc32	 (unsigned long, const unsigned char *, unsigned);
diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c
index 7f631c4..3ba9eae 100644
--- a/tools/env/fw_env_main.c
+++ b/tools/env/fw_env_main.c
@@ -42,16 +42,24 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
+#include <getopt.h>
 #include "fw_env.h"
 
 #define	CMD_PRINTENV	"fw_printenv"
 #define CMD_SETENV	"fw_setenv"
 
+static struct option long_options[] = {
+	{"script", required_argument, NULL, 's'},
+	{NULL, 0, NULL, 0}
+};
+
 int
 main(int argc, char *argv[])
 {
 	char *p;
 	char *cmdname = *argv;
+	char *script_file = NULL;
+	int c;
 
 	if ((p = strrchr (cmdname, '/')) != NULL) {
 		cmdname = p + 1;
@@ -65,9 +73,22 @@ main(int argc, char *argv[])
 		return (EXIT_SUCCESS);
 
 	} else if (strcmp(cmdname, CMD_SETENV) == 0) {
+		while ((c = getopt_long (argc, argv, "s:",
+				long_options, NULL)) != EOF) {
+			switch (c) {
+			case 's':
+				script_file = optarg;
+				break;
+			}
+		}
 
-		if (fw_setenv (argc, argv) != 0)
-			return (EXIT_FAILURE);
+		if (!script_file) {
+			if (fw_setenv(argc, argv) != 0)
+				return EXIT_FAILURE;
+		} else {
+			if (fw_parse_script(script_file) != 0)
+				return EXIT_FAILURE;
+		}
 
 		return (EXIT_SUCCESS);
 
-- 
1.6.3.3

[U-Boot] [PATCH V2] Tools: set multiple variable with fw_setenv utility

[Peter Tyser]

Hi Stefano,

On Sun, 2010-05-23 at 16:29 +0200, Stefano Babic wrote:
> Add a sort of batch mode to fw_setenv, allowing to set
> multiple variables in one shot, without updating the flash after
> each set as now. It is added the possibility to pass
> a config file with a list of pairs <variable, value> to be set,
> separated by a TAB character.

It'd be nice to document the new functionality you're adding.  Any
interest in adding a runtime usage message?  It seems like that'd be
pretty useful.  At a minimum it would be nice to update the description
in fw_env_main.c to reflect the changes you're making and/or update the
README.

> -int fw_setenv (int argc, char *argv[])
> +int fw_env_open(void)
>  {
> -	int i, len;
> -	char *env, *nxt;
> -	char *oldval = NULL;
> -	char *name;
> +	return env_init();
> +}

Is there a reason to keep fw_env_open around?  It looks like just a call
to env_init() after the changes above?

Best,
Peter

[U-Boot] [PATCH V2] Tools: set multiple variable with fw_setenv utility

[Stefano Babic]

Peter Tyser wrote:
> Hi Stefano,
> 

Hi Peter,

> It'd be nice to document the new functionality you're adding.  Any
> interest in adding a runtime usage message?  It seems like that'd be
> pretty useful.  At a minimum it would be nice to update the description
> in fw_env_main.c to reflect the changes you're making and/or update the
> README.

Agree. I think I will add a -h (help) option in fw_env_main.c, as you
suggest, describing in details how to call the utility.

> Is there a reason to keep fw_env_open around?  It looks like just a call
> to env_init() after the changes above?

The reason is to provide a classical file I/O interface, allowing to use
these utilities as library, too. This is consistent and more
comprehensive: the API is realized by fw_env_open, fw_env_write and
fw_env_close.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V3] Tools: set multiple variable with fw_setenv utility

[Stefano Babic]

Add a sort of batch mode to fw_setenv, allowing to set
multiple variables in one shot, without updating the flash after
each set as now. It is added the possibility to pass
a config file with a list of pairs <variable, value> to be set,
separated by a TAB character.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 tools/env/fw_env.c      |  269 +++++++++++++++++++++++++++++++++++++++--------
 tools/env/fw_env.h      |    4 +
 tools/env/fw_env_main.c |   67 +++++++++++--
 3 files changed, 288 insertions(+), 52 deletions(-)

Changes since V2:

Peter Tyser:
	- document new feature (help option)
	- replace env_init with fw_env_open

Myself:
	- Check environment overflow was done twice
	- Minor changes (code styling)
	- Added function prototypes in header file
	- fclose was not called

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index a46205d..04f3bf0 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -45,8 +45,7 @@
 
 #include "fw_env.h"
 
-#define	CMD_GETENV	"fw_printenv"
-#define	CMD_SETENV	"fw_setenv"
+#define WHITESPACE(c) ((c == '\t') || (c == ' '))
 
 #define min(x, y) ({				\
 	typeof(x) _min1 = (x);			\
@@ -210,7 +209,6 @@ static char default_environment[] = {
 
 static int flash_io (int mode);
 static char *envmatch (char * s1, char * s2);
-static int env_init (void);
 static int parse_config (void);
 
 #if defined(CONFIG_FILE)
@@ -225,6 +223,22 @@ static inline ulong getenvsize (void)
 	return rc;
 }
 
+static char *fw_string_blank(char *s, int noblank)
+{
+	int i;
+	int len = strlen(s);
+
+	for (i = 0; i < len; i++, s++) {
+		if ((noblank && !WHITESPACE(*s)) ||
+			(!noblank && WHITESPACE(*s)))
+			break;
+	}
+	if (i == len)
+		return NULL;
+
+	return s;
+}
+
 /*
  * Search the environment for a variable.
  * Return the value, if found, or NULL, if not found.
@@ -233,7 +247,7 @@ char *fw_getenv (char *name)
 {
 	char *env, *nxt;
 
-	if (env_init ())
+	if (fw_env_open())
 		return NULL;
 
 	for (env = environment.data; *env; env = nxt + 1) {
@@ -264,7 +278,7 @@ int fw_printenv (int argc, char *argv[])
 	int i, n_flag;
 	int rc = 0;
 
-	if (env_init ())
+	if (fw_env_open())
 		return -1;
 
 	if (argc == 1) {		/* Print all env variables  */
@@ -327,30 +341,34 @@ int fw_printenv (int argc, char *argv[])
 	return rc;
 }
 
-/*
- * Deletes or sets environment variables. Returns -1 and sets errno error codes:
- * 0	  - OK
- * EINVAL - need@least 1 argument
- * EROFS  - certain variables ("ethaddr", "serial#") cannot be
- *	    modified or deleted
- *
- */
-int fw_setenv (int argc, char *argv[])
+int fw_env_close(void)
 {
-	int i, len;
-	char *env, *nxt;
-	char *oldval = NULL;
-	char *name;
+	/*
+	 * Update CRC
+	 */
+	*environment.crc = crc32(0, (uint8_t *) environment.data, ENV_SIZE);
 
-	if (argc < 2) {
-		errno = EINVAL;
-		return -1;
+	/* write environment back to flash */
+	if (flash_io(O_RDWR)) {
+		fprintf(stderr,
+			"Error: can't write fw_env to flash\n");
+			return -1;
 	}
 
-	if (env_init ())
-		return -1;
+	return 0;
+}
 
-	name = argv[1];
+
+/*
+ * Set/Clear a single variable in the environment.
+ * This is called in sequence to update the environment
+ * in RAM without updating the copy in flash after each set
+ */
+int fw_env_write(char *name, char *value)
+{
+	int len;
+	char *env, *nxt;
+	char *oldval = NULL;
 
 	/*
 	 * search if variable with this name already exists
@@ -358,7 +376,7 @@ int fw_setenv (int argc, char *argv[])
 	for (nxt = env = environment.data; *env; env = nxt + 1) {
 		for (nxt = env; *nxt; ++nxt) {
 			if (nxt >= &environment.data[ENV_SIZE]) {
-				fprintf (stderr, "## Error: "
+				fprintf(stderr, "## Error: "
 					"environment not terminated\n");
 				errno = EINVAL;
 				return -1;
@@ -396,8 +414,8 @@ int fw_setenv (int argc, char *argv[])
 	}
 
 	/* Delete only ? */
-	if (argc < 3)
-		goto WRITE_FLASH;
+	if (!value || !strlen(value))
+		return 0;
 
 	/*
 	 * Append new definition@the end
@@ -411,41 +429,202 @@ int fw_setenv (int argc, char *argv[])
 	 */
 	len = strlen (name) + 2;
 	/* add '=' for first arg, ' ' for all others */
-	for (i = 2; i < argc; ++i) {
-		len += strlen (argv[i]) + 1;
-	}
+	len += strlen(value) + 1;
+
 	if (len > (&environment.data[ENV_SIZE] - env)) {
 		fprintf (stderr,
 			"Error: environment overflow, \"%s\" deleted\n",
 			name);
 		return -1;
 	}
+
 	while ((*env = *name++) != '\0')
 		env++;
+	*env = '=';
+	while ((*++env = *value++) != '\0')
+		;
+
+	/* end is marked with double '\0' */
+	*++env = '\0';
+
+	return 0;
+}
+
+/*
+ * Deletes or sets environment variables. Returns -1 and sets errno error codes:
+ * 0	  - OK
+ * EINVAL - need at least 1 argument
+ * EROFS  - certain variables ("ethaddr", "serial#") cannot be
+ *	    modified or deleted
+ *
+ */
+int fw_setenv(int argc, char *argv[])
+{
+	int i, len;
+	char *name;
+	char *value = NULL;
+	char *tmpval = NULL;
+
+	if (argc < 2) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (fw_env_open()) {
+		fprintf(stderr, "Error: environment not initialized\n");
+		return -1;
+	}
+
+	name = argv[1];
+
+	len = strlen(name) + 2;
+	for (i = 2; i < argc; ++i)
+		len += strlen(argv[i]) + 1;
+
+	/* Allocate enough place to the data string */
 	for (i = 2; i < argc; ++i) {
 		char *val = argv[i];
-
-		*env = (i == 2) ? '=' : ' ';
-		while ((*++env = *val++) != '\0');
+		if (!value) {
+			value = (char *)malloc(len - strlen(name));
+			if (!value) {
+				fprintf(stderr,
+				"Cannot malloc %u bytes: %s\n",
+				len - strlen(name), strerror(errno));
+				return -1;
+			}
+			memset(value, 0, len - strlen(name));
+			tmpval = value;
+		}
+		if (i != 2)
+			*tmpval++ = ' ';
+		while (*val != '\0')
+			*tmpval++ = *val++;
 	}
 
-	/* end is marked with double '\0' */
-	*++env = '\0';
+	fw_env_write(name, value);
 
-  WRITE_FLASH:
+	if (value)
+		free(value);
 
-	/*
-	 * Update CRC
-	 */
-	*environment.crc = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
+	return fw_env_close();
+}
 
-	/* write environment back to flash */
-	if (flash_io (O_RDWR)) {
-		fprintf (stderr, "Error: can't write fw_env to flash\n");
+/*
+ * Parse  a file  and configure the u-boot variables.
+ * The script file has a very simple format, as follows:
+ *
+ * Each line has a couple with name, value:
+ * <white spaces>variable_name<white spaces>variable_value
+ *
+ * Both variable_name and variable_value are interpreted as strings.
+ * Any character after <white spaces> and before ending \r\n is interpreted
+ * as variable's value (no comment allowed on these lines !)
+ *
+ * Comments are allowed if the first character in the line is #
+ *
+ * Returns -1 and sets errno error codes:
+ * 0	  - OK
+ * -1     - Error
+ */
+int fw_parse_script(char *fname)
+{
+	FILE *fp;
+	char dump[1024];	/* Maximum line length in the file */
+	char *name;
+	char *val;
+	int lineno = 0;
+	int len;
+	int ret = 0;
+
+	if (fw_env_open()) {
+		fprintf(stderr, "Error: environment not initialized\n");
 		return -1;
 	}
 
-	return 0;
+	if (strcmp(fname, "-") == 0)
+		fp = stdin;
+	else {
+		fp = fopen(fname, "r");
+		if (fp == NULL) {
+			fprintf(stderr, "I cannot open %s for reading\n",
+				 fname);
+			return -1;
+		}
+	}
+
+	while (fgets(dump, sizeof(dump), fp)) {
+		lineno++;
+		len = strlen(dump);
+
+		/*
+		 * Read a whole line from the file. If the line is too long
+		 * or is not terminated, reports an error and exit.
+		 */
+		if (dump[len - 1] != '\n') {
+			fprintf(stderr,
+			"Line %d not corrected terminated or too long\n",
+				lineno);
+			ret = -1;
+			break;
+		}
+
+		/* Drop ending line feed / carriage return */
+		while (len > 0 && (dump[len - 1] == '\n' ||
+				dump[len - 1] == '\r')) {
+			dump[len - 1] = '\0';
+			len--;
+		}
+
+		/* Skip comment or empty lines */
+		if ((len == 0) || dump[0] == '#')
+			continue;
+
+		/*
+		 * Search for variable's name,
+		 * remove leading whitespaces
+		 */
+		name = fw_string_blank(dump, 1);
+		if (!name)
+			continue;
+
+		/* The first white space is the end of variable name */
+		val = fw_string_blank(name, 0);
+		len = strlen(name);
+		if (val) {
+			*val++ = '\0';
+			if ((val - name) < len)
+				val = fw_string_blank(val, 1);
+			else
+				val = NULL;
+		}
+
+#ifdef DEBUG
+		fprintf(stderr, "Setting %s : %s\n",
+			name, val ? val : " removed");
+#endif
+
+		/*
+		 * If there is an error setting a variable,
+		 * try to save the environment and returns an error
+		 */
+		if (fw_env_write(name, val)) {
+			fprintf(stderr,
+			"fw_env_write returns with error : %s\n",
+				strerror(errno));
+			ret = -1;
+			break;
+		}
+
+	}
+
+	/* Close file if not stdin */
+	if (strcmp(fname, "-") != 0)
+		fclose(fp);
+
+	ret |= fw_env_close();
+
+	return ret;
+
 }
 
 /*
@@ -880,7 +1059,7 @@ static char *envmatch (char * s1, char * s2)
 /*
  * Prevent confusion if running from erased flash memory
  */
-static int env_init (void)
+int fw_env_open(void)
 {
 	int crc0, crc0_ok;
 	char flag0;
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
index c04da54..8130fa1 100644
--- a/tools/env/fw_env.h
+++ b/tools/env/fw_env.h
@@ -50,5 +50,9 @@
 extern int   fw_printenv(int argc, char *argv[]);
 extern char *fw_getenv  (char *name);
 extern int fw_setenv  (int argc, char *argv[]);
+extern int fw_parse_script(char *fname);
+extern int fw_env_open(void);
+extern int fw_env_write(char *name, char *value);
+extern int fw_env_close(void);
 
 extern unsigned	long  crc32	 (unsigned long, const unsigned char *, unsigned);
diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c
index 7f631c4..82116b4 100644
--- a/tools/env/fw_env_main.c
+++ b/tools/env/fw_env_main.c
@@ -42,34 +42,87 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
+#include <getopt.h>
 #include "fw_env.h"
 
 #define	CMD_PRINTENV	"fw_printenv"
 #define CMD_SETENV	"fw_setenv"
 
+static struct option long_options[] = {
+	{"script", required_argument, NULL, 's'},
+	{"help", no_argument, NULL, 'h'},
+	{NULL, 0, NULL, 0}
+};
+
+void usage(void)
+{
+
+	fprintf(stderr, "fw_printenv/fw_setenv, "
+		"a command line interface to U-Boot environment\n\n"
+		"usage:\tfw_printenv\n"
+		"\tfw_setenv [variable name] [variable value]\n"
+		"\tfw_setenv -s [ file ]\n"
+		"\tfw_setenv -s - < [ file ]\n\n"
+		"The file passed as argument contains only pairs "
+		"name / value\n"
+		"Example:\n"
+		"# Any line starting with # is treated as comment\n"
+		"\n"
+		"\t      netdev         eth0\n"
+		"\t      kernel_addr    400000\n"
+		"\t      var1\n"
+		"\t      var2          The quick brown fox jumps over the "
+		"lazy dog\n"
+		"\n"
+		"A variable without value will be dropped. It is possible\n"
+		"to put any number of spaces between the fields, but any\n"
+		"space inside the value is treated as part of the value "
+		"itself.\n\n"
+	);
+}
+
 int
 main(int argc, char *argv[])
 {
 	char *p;
 	char *cmdname = *argv;
+	char *script_file = NULL;
+	int c;
 
 	if ((p = strrchr (cmdname, '/')) != NULL) {
 		cmdname = p + 1;
 	}
 
+	while ((c = getopt_long (argc, argv, "s:h",
+		long_options, NULL)) != EOF) {
+		switch (c) {
+		case 's':
+			script_file = optarg;
+			break;
+		case 'h':
+			usage();
+			return EXIT_SUCCESS;
+		}
+	}
+
+
 	if (strcmp(cmdname, CMD_PRINTENV) == 0) {
 
 		if (fw_printenv (argc, argv) != 0)
-			return (EXIT_FAILURE);
+			return EXIT_FAILURE;
 
-		return (EXIT_SUCCESS);
+		return EXIT_SUCCESS;
 
 	} else if (strcmp(cmdname, CMD_SETENV) == 0) {
+		if (!script_file) {
+			if (fw_setenv(argc, argv) != 0)
+				return EXIT_FAILURE;
+		} else {
+			if (fw_parse_script(script_file) != 0)
+				return EXIT_FAILURE;
+		}
 
-		if (fw_setenv (argc, argv) != 0)
-			return (EXIT_FAILURE);
-
-		return (EXIT_SUCCESS);
+		return EXIT_SUCCESS;
 
 	}
 
@@ -77,5 +130,5 @@ main(int argc, char *argv[])
 		"Identity crisis - may be called as `" CMD_PRINTENV
 		"' or as `" CMD_SETENV "' but not as `%s'\n",
 		cmdname);
-	return (EXIT_FAILURE);
+	return EXIT_FAILURE;
 }
-- 
1.6.3.3

[U-Boot] [PATCH V3] Tools: set multiple variable with fw_setenv utility

[Wolfgang Denk]

Dear Stefano Babic,

In message <1274695696-8606-1-git-send-email-sbabic@denx.de> you wrote:
> Add a sort of batch mode to fw_setenv, allowing to set
> multiple variables in one shot, without updating the flash after
> each set as now. It is added the possibility to pass
> a config file with a list of pairs <variable, value> to be set,
> separated by a TAB character.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  tools/env/fw_env.c      |  269 +++++++++++++++++++++++++++++++++++++++--------
>  tools/env/fw_env.h      |    4 +
>  tools/env/fw_env_main.c |   67 +++++++++++--
>  3 files changed, 288 insertions(+), 52 deletions(-)

Applied to "next" branch. Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If a computer can't directly address all the RAM you can  use,  it's
just a toy."         - anonymous comp.sys.amiga posting, non-sequitir