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

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

On 03/13/2014 08:49 PM, Stephen Warren wrote:
> On 03/13/2014 11:28 AM, Przemyslaw Marczak wrote:
>> 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
>
>>>> @@ -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 ret has nothing to do with it; there's no code in this function
> which prints messges based on ret.
>
> The code should be:
>
> e = getenv(s);
> if (e == NULL) {
> 	printf("Env var '%s' not set; using random UUID\n", str);
> 	// generate random UUID here
> 	if (UUID generation failed) {
> 		printf("ERROR generating random UUID\n");
> 		return -1;
> 	}
Take a note that gen_rand_uuid_str() doesn't return any error. So this 
error is not true.

> }
>
> With the code in your patch, the following happens:
>
> * Env var is set: Nothing printed.
And env var is not changed - and this is as before.
>
> * Env var not set, but problem during UUID generation: "%s unset. " is
> printed without a \n at the end. The error does get propagated back tot
> he caller, which might print a message with \n at the end, but you
> shouldn't place that kind of requirement on the caller. Rather, this
> function should be fully self-contained.
>
> * Env var set, and UUID generated OK: "%s unset. Setting to random.\n"
> 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.
>
> I don't think convenience of coding or the size of the patch is
> justification for writing values to the user's environment when they
> didn't ask for it. What if they run saveenv after executing this
> function, yet they specifically want the environment variables unset, so
> that a random UUID is generated each time this function/command is run?
>
Actually I don't understand what is the problem. What is the difference 
when user set manually $uuid_gpt_* or use generated by gpt command if 
next he write "save", in both situations variables are saved. I don't 
think it is a problem.

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