[U-Boot] [PATCH 1/2] lib: uuid: add function to generate UUID version 4

[Stephen Warren]

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.