[U-Boot] [PATCH V2 2/3] lib: uuid: add functions to generate UUID version 4

[Stephen Warren]

On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:
> This patch adds support to generate UUID (Universally Unique Identifier)
> in version 4 based on RFC4122, which is randomly.
> 
> Source: https://www.ietf.org/rfc/rfc4122.txt
> 
> Changes:
> - add new config: CONFIG_RANDOM_UUID: compile uuid.c and rand.c
> 
> Update files:
> - include/common.h
> - lib/Makefile
> - lib/uuid.c

The patch already contains the list of changed files; there's little
point duplicating this in the patch description.

> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
> cc: Stephen Warren <swarren at nvidia.com>
> cc: trini at ti.com

s/cc/Cc/

> diff --git a/lib/Makefile b/lib/Makefile
> index 70962b2..64a430f 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -59,10 +59,12 @@ obj-y += linux_string.o
>  obj-$(CONFIG_REGEX) += slre.o
>  obj-y += string.o
>  obj-y += time.o
> +obj-y += vsprintf.o

Moving vsprintf.o seems entirely unrelated to this patch. If you want to
clean that up, it should be a separate patch.

>  obj-$(CONFIG_TRACE) += trace.o
>  obj-$(CONFIG_BOOTP_PXE) += uuid.o
>  obj-$(CONFIG_PARTITION_UUIDS) += uuid.o
> -obj-y += vsprintf.o
> +obj-$(CONFIG_RANDOM_UUID) += uuid.o
> +obj-$(CONFIG_RANDOM_UUID) += rand.o
>  obj-$(CONFIG_RANDOM_MACADDR) += rand.o

I really hope listing the same object multiple times in obj-y is OK.

Why not sort the lines you added so based on the config variable, so

>  obj-$(CONFIG_RANDOM_MACADDR) += rand.o
> +obj-$(CONFIG_RANDOM_UUID) += rand.o

rather than:

> +obj-$(CONFIG_RANDOM_UUID) += rand.o
>  obj-$(CONFIG_RANDOM_MACADDR) += rand.o

> diff --git a/lib/uuid.c b/lib/uuid.c
> index 803bdcd..c0218ba 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -7,6 +7,29 @@
>  #include <linux/ctype.h>
>  #include <errno.h>
>  #include <common.h>
> +#include <part_efi.h>
> +#include <malloc.h>
> +
> +#define UUID_STR_LEN			36
> +#define UUID_STR_BYTE_LEN		37

If UUID_STR_BYTE_LEN is the length in bytes, what units is UUID_STR_LEN
in? I suppose the difference is the NULL terminator, but the names don't
make that clear at all. I would suggest not defining UUID_STR_BYTE_LEN
at all, but rather just writing "+ 1" at the appropriate place in the
source code.

> +#define UUID_BIN_BYTE_LEN		16

Also, s/_BYTE// there.

> +#define UUID_VERSION_CLEAR_BITS		0x0fff

s/CLEAR_BITS/MASK/

> +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];
> +};

Is that structure definition endianness-safe?

> +/*
> + * gen_rand_uuid() - this function generates 16 bytes len UUID V4 (randomly)
> + *                   and stores it at a given pointer.

I think "this function generates a random binary UUID v4, and stores it
into the memory pointed at by the supplied pointer, which must be 16
bytes in size" would be better.

> +void gen_rand_uuid(unsigned char *uuid_bin)
> +{
> +	struct uuid *uuid = (struct uuid *)uuid_bin;
> +	unsigned int *ptr = (unsigned int *)uuid_bin;
> +	int i;
> +
> +	if (!uuid_bin)
> +		return;

I think you should either (a) assume NULL will never be passed to this
function, or (b) return an error code if it happens. Silently failing to
do anything doesn't make sense.

> +	memset(uuid_bin, 0x0, sizeof(struct uuid));
> +
> +	/* Set all fields randomly */
> +	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
> +		*(ptr + i) = rand();

If the entire thing is filled randomly, why memset() the struct?


> +/*
> + * gen_rand_uuid_str() - this function generates UUID v4 (randomly)
> + * into 36 character hexadecimal ASCII string representation of a 128-bit
> + * (16 octets) UUID (Universally Unique Identifier) version 4 based on RFC4122
> + * and stores it at a given pointer.

There's a lot of duplication in that description. I think "this function
generates a random string-formatted UUID v4, and stores it into the
memory pointed at by the supplied pointer, which must be 37 bytes in
size" would be better.

> +void gen_rand_uuid_str(char *uuid_str)
> +{
> +	unsigned char uuid_bin[UUID_BIN_BYTE_LEN];
> +
> +	if (!uuid_str)
> +		return;

Again, I would drop that error-checking.