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

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello,
Sorry for silent, but I've had some other work.
I agree with yours previous comments and those will apply to v3 but I 
don't agree with few comments to this patch.

On 03/10/2014 06:44 PM, Stephen Warren wrote:
> On 03/05/2014 09:45 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)
>> +static int extract_env(const char *str, char **env)
>>   {
>> +	int ret = -1;
>>   	char *e, *s;
>> +	char uuid_str[37];
>>
>>   	if (!str || strlen(str) < 4)
>>   		return -1;
>> @@ -43,16 +45,25 @@ 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);
>> +			gen_rand_uuid_str(uuid_str);
>> +			setenv(s, uuid_str);
>> +

In this place ret is "-1".

>> +			e = getenv(s);
>> +			if (e) {
>> +				puts("Setting to random.\n");
>
> Shouldn't this be printed right after the "if (e == NULL)" check above?
> That's where the decision is made to generate a random UUID.
>
> Here, "if (!e)", the code should return an error.
>
If (!e) then ret is still "-1".
If (e) then ret = 0 and proper info is printed.

> But, I still don't like changing the environment. Why can't the above
> few lines be:
>
> + 			gen_rand_uuid_str(uuid_str);
> +			e = uuid_str;

Such solution needs more code rewriting and breaking some existing 
cmd_gpt design. "e" is used outside this function but uuid_str is local 
here. I don't like to make it static.
Using getenv and return its pointer will work the same as previous.

Please note that variables set by user are not overwritten here so this 
code will only set null uuid env variables. Moreover user can see after 
gpt command that environment is the same with mmc part shows, I think it 
is useful instead of situation when uuid is set but not present in 
environment.

>
>> diff --git a/doc/README.gpt b/doc/README.gpt
>
>>   "uuid" program is recommended to generate UUID string. Moreover it can decode
>>   (-d switch) passed in UUID string. It can be used to generate partitions UUID
>>   passed to u-boot environment variables.
>> +If each partition "uuid" no exists then it will be randomly generated.
>
> "If each" means "if all of them", implying that it's an all-or-nothing
> solution, and the random generation only happens of none of the UUIDs
> were supplied, not on a UUID-by-UUID basis. So, s/each/a/ or s/each/any/.
>
Ok :)


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