[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 at 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