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