[U-Boot] [PATCH 2/2] cmd:gpt: randomly generate each partition uuid if undefined

[Przemyslaw Marczak]

Hello again,

On 02/28/2014 06:03 PM, Stephen Warren wrote:
> On 02/28/2014 08:18 AM, Przemyslaw Marczak wrote:
>> Changes:
>> - randomly generate each partition uuid if undefined
>> - print info about generated uuid
>> - save environment on gpt write success
>> - update doc/README.gpt
>
>> diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c
>
>>   static char extract_env(const char *str, char **env)
>>   {
>> +	int ret = -1;
>
> Why does the function return char not int? At least the type of ret
> should match the return type of the function.
>

Right notice, this should be fixed.

> There's no need to introduce a ret variable anyway; just don't delete
> the return statements that are already in the function.
>

But I need to move "free(s)" so I can't leave current return statements. 
Other way I need to put "free(s)" in few places.

>> @@ -43,16 +44,23 @@ static char extract_env(const char *str, char **env)
>>   		memset(s + strlen(s) - 1, '\0', 1);
>>   		memmove(s, s + 2, strlen(s) - 1);
>>   		e = getenv(s);
>> -		free(s);
>>   		if (e == NULL) {
>> -			printf("Environmental '%s' not set\n", str);
>> -			return -1; /* env not set */
>> +			printf("%s unset. ", str);
>> +			e = get_uuid_str();
>> +			if (e) {
>> +				printf("Setting to: %s\n", e);
>> +				setenv(s, e);

And here I forget about free(e)...

>
> Why should the environment variable be set? I rather dislike commands
> that randomly set environment variables as an implicit side-effect.
>

Actually automatically generated uuids was the main purpose of this 
patches. Setting each env variable in this place was the most easy way 
to make this without a lot of duplicated code.

Why do you treat it like a side-effect?
If user wants have own generated uuids - then he can manually set env 
variables like "uuid_gpt_disk".
This actually is not changed - when uuid env is set then it will be used 
like in current version of code.
When user can't generate uuids or just wants to have it automatically 
generated then my code do this job.

> It'd be far better if this function simply wasn't modified, but rather
> the user was provided with a function to explicitly set an environment
> variable to a randomly generated GPT. That way the user/script would be
> in control. Something like:
>
> $ gen_random_uuid env_var_name
>

I understand that this is very important code, but setting each val 
manually with random uuid actually will not change anything - user still 
needs to do something.

The other way is to provide a function for parse e.g $partitions but 
then it will be a code duplication. The main job is done by 
set_gpt_info() so this is why I modified this code.

>> @@ -299,8 +307,17 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>   			return CMD_RET_FAILURE;
>>   		}
>>
>> -		if (gpt_default(blk_dev_desc, argv[4]))
>> +		puts("Writing GPT: ");
>> +
>> +		ret = gpt_default(blk_dev_desc, argv[4]);
>> +		if (!ret) {
>> +			puts("success!\n");
>> +			saveenv();
>
> Uggh. Definitely don't save the environment behind the user's back.
> There is no reason to believe that's safe. What if the user had added
> some temporary changes to their environment that they didn't want saved?
> This kind of logic belongs in scripts, not code.
>
>

The one and only reason for put saveenv() here was that if uuids are 
randomly generated or even just are in environment then I can be sure 
that next gpt write (e.g. in case of overwrite sector 0 by mistake) is 
using the same uuids values.

Maybe saveenv() in this place is not the best idea but in other side 
user actually uses this command just once.

Thank you
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com