From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 23 Feb 2015 15:42:13 +0100 Subject: [U-Boot] [PATCH] gpt: support random UUIDs without setting environment variables In-Reply-To: References: <1422287058-24399-1-git-send-email-robh@kernel.org> <54E4D068.4080406@samsung.com> Message-ID: <54EB3C45.5090704@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello, On 02/22/2015 11:46 PM, Rob Herring wrote: > On Wed, Feb 18, 2015 at 11:48 AM, Przemyslaw Marczak > 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 >>> --- >>> 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 Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com