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

[Stefano Babic <sbabic@denx.de>]

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
=====================================================================