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. > @@ -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. > 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. > +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? > /* 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. Also, not everyone who defines CONFIG_PARTITION_UUIDs needs the new get_uuid_str() function, and hence not everyone needs rand() etc. > 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. > 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. > -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. > +/* > + * 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. 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. > +{ > + 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? > + /* 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). > + 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. > + if (!uuid_str_valid(uuid_str)) { > + error("Invalid UUID string"); > + return NULL; > + } Isn't that code already part of uuid_bin_to_str()? > + /* 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot