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

[Stephen Warren]

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;
	}
}

With the code in your patch, the following happens:

* Env var is set: Nothing printed.

* 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?