[U-Boot] [PATCH] Tools: set multiple variable with fw_setenv utility
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 at 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 at 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);
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
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
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
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
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
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 at 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 at 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 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 (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);
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
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
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 at 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 at 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; }
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
participants (3)
-
Peter Tyser -
Stefano Babic -
Wolfgang Denk