[U-Boot] [PATCH] gpt: support random UUIDs without setting environment variables

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

Hello,

On 02/22/2015 11:46 PM, Rob Herring wrote:
> On Wed, Feb 18, 2015 at 11:48 AM, Przemyslaw Marczak
> <p.marczak@samsung.com> wrote:
>> Hello Rob,
>>
>> Sorry for delay.
>>
>> On 01/26/2015 04:44 PM, Rob Herring wrote:
>>>
>>> Currently, an environment variable must be used to store the randomly
>>> generated UUID for each partition. This is not necessary, so make storing
>>> the UUID optional. Now passing uuid_disk and uuid are optional when random
>>> UUIDs are enabled.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>    common/cmd_gpt.c | 48 ++++++++++++++++++++++++++++++------------------
>>>    doc/README.gpt   |  8 +++++---
>>>    2 files changed, 35 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c
>>> index 75df3fe..c56fe15 100644
>>> --- a/common/cmd_gpt.c
>>> +++ b/common/cmd_gpt.c
>>> @@ -154,17 +154,24 @@ static int set_gpt_info(block_dev_desc_t *dev_desc,
>>>
>>>          /* extract disk guid */
>>>          s = str;
>>> -       tok = strsep(&s, ";");
>>> -       val = extract_val(tok, "uuid_disk");
>>> +       val = extract_val(str, "uuid_disk");
>>>          if (!val) {
>>
>>
>> The code below is not required, since the same thing is done inside the
>> extract_env() function.
>
> Okay.
>
>>
>>
>>> +#ifdef CONFIG_RANDOM_UUID
>>> +               *str_disk_guid = malloc(UUID_STR_LEN + 1);
>>> +               gen_rand_uuid_str(*str_disk_guid, UUID_STR_FORMAT_STD);
>>> +#else
>>>                  free(str);
>>>                  return -2;
>>> +#endif
>>> +       } else {
>>> +               val = strsep(&val, ";");
>>> +               if (extract_env(val, &p))
>>> +                       p = val;
>>> +               *str_disk_guid = strdup(p);
>>> +               free(val);
>>> +               /* Move s to first partition */
>>> +               strsep(&s, ";");
>>>          }
>>> -       if (extract_env(val, &p))
>>> -               p = val;
>>> -       *str_disk_guid = strdup(p);
>>> -       free(val);
>>> -
>>>          if (strlen(s) == 0)
>>>                  return -3;
>>>
>>> @@ -192,20 +199,25 @@ static int set_gpt_info(block_dev_desc_t *dev_desc,
>>>
>>>                  /* uuid */
>>>                  val = extract_val(tok, "uuid");
>>> -               if (!val) { /* 'uuid' is mandatory */
>>> -                       errno = -4;
>>> -                       goto err;
>>> -               }
>>> -               if (extract_env(val, &p))
>>> -                       p = val;
>>> -               if (strlen(p) >= sizeof(parts[i].uuid)) {
>>> -                       printf("Wrong uuid format for partition %d\n", i);
>>> +               if (!val) {
>>
>>
>> The same in this place - code duplication.
>>
>>
>>> +                       /* 'uuid' is optional if random uuid's are enabled
>>> */
>>> +#ifdef CONFIG_RANDOM_UUID
>>> +                       gen_rand_uuid_str(parts[i].uuid,
>>> UUID_STR_FORMAT_STD);
>>> +#else
>>>                          errno = -4;
>>>                          goto err;
>>> +#endif
>>> +               } else {
>>> +                       if (extract_env(val, &p))
>>> +                               p = val;
>>> +                       if (strlen(p) >= sizeof(parts[i].uuid)) {
>>> +                               printf("Wrong uuid format for partition
>>> %d\n", i);
>>> +                               errno = -4;
>>> +                               goto err;
>>> +                       }
>>> +                       strcpy((char *)parts[i].uuid, p);
>>> +                       free(val);
>>>                  }
>>> -               strcpy((char *)parts[i].uuid, p);
>>> -               free(val);
>>> -
>>>                  /* name */
>>>                  val = extract_val(tok, "name");
>>>                  if (!val) { /* name is mandatory */
>>> diff --git a/doc/README.gpt b/doc/README.gpt
>>> index ec0156d..59fdeeb 100644
>>> --- a/doc/README.gpt
>>> +++ b/doc/README.gpt
>>> @@ -157,11 +157,13 @@ To restore GUID partition table one needs to:
>>>         "partitions=uuid_disk=${uuid_gpt_disk};name=${uboot_name},
>>>          size=${uboot_size},uuid=${uboot_uuid};"
>>>
>>> -   Fields 'name', 'size' and 'uuid' are mandatory for every partition.
>>> +   The fields 'name' and 'size' are mandatory for every partition.
>>>       The field 'start' is optional.
>>>
>>> -   option: CONFIG_RANDOM_UUID
>>> -   If any partition "UUID" no exists then it is randomly generated.
>>> +   The fields 'uuid' and 'uuid_disk' are optional if CONFIG_RANDOM_UUID
>>> is
>>> +   enabled. A random uuid will be used if omitted or they point to an
>>> empty/
>>> +   non-existent environment variable. The environment variable will be
>>> set to
>>> +   the generated UUID.
>>
>>
>> The things from the above comment are implemented at present in mainline.
>
> Partially yes, but as presently worded it did not make sense to me. It
> took me some time to figure out I had to define env variables for each
> uuid. So I expanded the text so the next person is not scratching
> their head to get this to work.
>

I see, the text which you add is clear for me.

>>
>>>
>>>    2. Define 'CONFIG_EFI_PARTITION' and 'CONFIG_CMD_GPT'
>>>
>>>
>>
>>
>> If you want drop saving the uuid to env, then you can do it by modify the
>> extract_env() function (diff):
>>
>> +#ifdef CONFIG_RANDOM_UUID_SKIP_SETENV
>> +               e = strdup(uuid_str);
>> +#else
>>                  setenv(s, uuid_str);
>> -
>>                  e = getenv(s);
>> +#endif
>>
>>
>> I would prefer introduce the new config like *SKIP_SETENV*, rather than drop
>> this feature at all, since somebody could use it.
>
> I did not drop the feature as both saving to env or not are supported
> and I see little reason to make it compile time, but I will look
> whether I can move some of this to extract_env.
>
> Rob
>

Ah, sorry, the topic of this patch was quite misleading.

At present, the random uuid is generated and saved to the env variable - 
I though, that you tried to drop the setenv routine.

Now I see that you want to drop the need to inserting the uuid variables 
into the ${partitions}.

Generating the random uuid was introduced to make the things easier.

The feature, which you want to introduce also looks like a benefit.

In this case the patch looks good.

Could you please add an example of $partitions without the uuid to the 
README.gpt file?

Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>

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