From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 03 Mar 2014 14:44:57 +0100 Subject: [U-Boot] [PATCH 1/2] lib: uuid: add function to generate UUID version 4 In-Reply-To: <5310BF65.6040603@wwwdotorg.org> References: <72629ef58556732156fadf26a2a48be85704f224.1393600504.git.p.marczak@samsung.com> <5310BF65.6040603@wwwdotorg.org> Message-ID: <53148759.5020001@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 Stephen, Thank you for review. On 02/28/2014 05:55 PM, Stephen Warren wrote: > On 02/28/2014 08:18 AM, Przemyslaw Marczak wrote: >> lib/uuid.c: >> Add get_uuid_str() - this function returns 36 character hexadecimal ASCII >> string representation of a 128-bit (16 octets) UUID (Universally Unique >> Identifier) version 4 based on RFC4122, which is randomly generated. >> >> Source: https://www.ietf.org/rfc/rfc4122.txt > >> diff --git a/disk/part_efi.c b/disk/part_efi.c > >> @@ -132,9 +113,11 @@ void print_part_efi(block_dev_desc_t * dev_desc) >> le64_to_cpu(gpt_pte[i].ending_lba), >> print_efiname(&gpt_pte[i])); >> printf("\tattrs:\t0x%016llx\n", gpt_pte[i].attributes.raw); >> - uuid_string(gpt_pte[i].partition_type_guid.b, uuid); >> + uuid_bin = (unsigned char *)gpt_pte[i].partition_type_guid.b; >> + uuid_bin_to_str(uuid_bin, uuid); > > I don't know why you need the uuid_bin temporary variable; you could > just as well do the cast as part of the function parameter. Not a big > deal though. > Just because the line was too long. >> @@ -182,7 +165,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part, > >> #ifdef CONFIG_PARTITION_UUIDS >> - uuid_string(gpt_pte[part - 1].unique_partition_guid.b, info->uuid); >> + uuid_bin_to_str(gpt_pte[part - 1].unique_partition_guid.b, info->uuid); >> #endif > > But you don't use a temporary here, for example. > Because this line doesn't exceeds 80 characters... >> diff --git a/include/common.h b/include/common.h > >> /* lib/uuid.c */ >> -void uuid_str_to_bin(const char *uuid, unsigned char *out); >> +char *get_uuid_str(void); > > See below; I think this prototype should be added in a separate patch. > Ok, will be changed. >> +int uuid_bin_to_str(unsigned char *uuid, char *str); > > Can this ever fail? If you're explicitly changing it to have a return > cdoe, why do none of the callers check the return code? > Actually it shouldn't, so I will change this return type to void. >> /* lib/rand.c */ >> #if defined(CONFIG_RANDOM_MACADDR) || \ >> defined(CONFIG_BOOTP_RANDOM_DELAY) || \ >> - defined(CONFIG_CMD_LINK_LOCAL) >> + defined(CONFIG_CMD_LINK_LOCAL) || \ >> + defined(CONFIG_PARTITION_UUIDS) > > This patch does two things: > > a) Refactor the UUID bin<->str code so that it's in a shared place > b) Add new code get_uuid_str(). > > I think this patch should only do (a), and (b) should be part of a > separate patch. As such, the hunk above should be separated out. Perhaps > (b) should be part of patch 2/2, or a new patch inserted between the two. > Ok, I will separate each change. > Also, not everyone who defines CONFIG_PARTITION_UUIDs needs the new > get_uuid_str() function, and hence not everyone needs rand() etc. > I understand but now this will be a part of UUID library so do you prefer to add proper #ifdef in code? #ifdef CONFIG_GENERATE_UUID char *get_uuid_str(void) { ... ... } #endif >> diff --git a/lib/Makefile b/lib/Makefile > >> +ifdef CONFIG_PARTITION_UUIDS >> +obj-y += rand.o >> +obj-y += uuid.o >> +endif > > That'd be better as: > > obj-$(CONFIG_PARTITION_UUIDS) rand.o > obj-$(CONFIG_PARTITION_UUIDS) uuid.o > > ... although the rand.o change should be in a separate patch. > Ok, it will be included in get_uuid_str() patch. >> diff --git a/lib/uuid.c b/lib/uuid.c > >> +#define UUID_STR_BYTE_LEN 37 >> + >> +#define UUID_VERSION_CLEAR_BITS 0x0fff >> +#define UUID_VERSION_SHIFT 12 >> +#define UUID_VERSION 0x4 >> + >> +#define UUID_VARIANT_CLEAR_BITS 0x3f >> +#define UUID_VARIANT_SHIFT 7 >> +#define UUID_VARIANT 0x1 >> + >> +struct uuid { >> + unsigned int time_low; >> + unsigned short time_mid; >> + unsigned short time_hi_and_version; >> + unsigned char clock_seq_hi_and_reserved; >> + unsigned char clock_seq_low; >> + unsigned char node[6]; >> +}; > > Most/all of that is support for get_uuid_str(), so should probably be > added in a separate patch. > OK. >> -void uuid_str_to_bin(const char *uuid, unsigned char *out) >> +int uuid_str_to_bin(char *uuid, unsigned char *out) >> { >> uint16_t tmp16; >> uint32_t tmp32; >> uint64_t tmp64; >> >> if (!uuid || !out) >> - return; >> + return -EINVAL; >> + >> + if (!uuid_str_valid(uuid)) >> + return -EINVAL; > > I'm not convinced it's useful to add this error-check; the code already > works or doesn't. Adding a unit-test to test/command_ut.c might be more > useful. > Right, this code is simple. Error check will be removed from here. >> +/* >> + * get_uuid_str() - this function returns pointer to 36 character hexadecimal >> + * ASCII string representation of a 128-bit (16 octets) UUID (Universally >> + * Unique Identifier) version 4 based on RFC4122. >> + * source: https://www.ietf.org/rfc/rfc4122.txt >> + * >> + * Layout of UUID Version 4: >> + * timestamp - 60-bit: time_low, time_mid, time_hi_and_version >> + * version - 4 bit (bit 4 through 7 of the time_hi_and_version) >> + * clock seq - 14 bit: clock_seq_hi_and_reserved, clock_seq_low >> + * variant: - bit 6 and 7 of clock_seq_hi_and_reserved >> + * node - 48 bit >> + * In this version all fields beside 4 bit version are randomly generated. >> + * >> + * @ret: pointer to 36 bytes len characters array >> + */ >> +char *get_uuid_str(void) > > This function name isn't particularly good; it gives no hint that it's > generating a random UUID. Perhaps generate_random_uuid_str() would be > better. What about this? /* To generate bin uuid */ void gen_rand_uuid(unsigned char *uuid) { if (!uuid) return; ... } > > Why does the function malloc the string, rather than writing to a > user-allocated buffer like uuid_bin_to_str()? That would be more > consistent with the other API, and simpler to code, and then couldn't > ever fail. So as in declaration above - user should pass allocated pointer. > >> +{ >> + struct uuid uuid; >> + char *uuid_str = NULL; >> + int *ptr = (int *)&uuid; >> + int i; >> + >> + uuid_str = malloc(UUID_STR_BYTE_LEN); >> + if (!uuid_str) { >> + error("uuid_str pointer is null"); > > More like allocation failed; the existing message implies that a NULL > pointer was passed into the function. Does error() tell you which > file/line/function the problem occurred in? > I agree with you - this was not good. >> + /* Set all fields randomly */ >> + for (i = 0; i < sizeof(uuid) / 4; i++) >> + *(ptr + i) = rand(); > > Replace "4" with sizeof(int) or even better, sizeof(*ptr). > Ok. >> + uuid_bin_to_str((unsigned char *)&uuid, uuid_str); > > Why not generate a random binary UUID; it's quite possible the caller > wants a binary version and would just have to undo this call. You could > create separate generate_random_uuid_bin() and provide a simple wrapper > generate_random_uuid_str() that called it. Ok, will be added. > >> + if (!uuid_str_valid(uuid_str)) { >> + error("Invalid UUID string"); >> + return NULL; >> + } > > Isn't that code already part of uuid_bin_to_str()? Right, this is duplication... > >> + /* Put end of string */ >> + uuid_str[UUID_STR_BYTE_LEN - 1] = '\0'; > > If it isn't already, uuid_bin_to_str() should be doing that. > I will improve those changes in the next version. Thank you for comments. -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com