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

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello again,

On 03/10/2014 06:37 PM, Stephen Warren wrote:
> 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@samsung.com>
>> cc: Stephen Warren <swarren@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?
>

UUID format is big-endian.
Actually for version 4 it doesn't matter because of it is random, and 
RFC says that version and variant are the most significant bits of 
proper structure field. In this code version and variant mask are stored 
at most significant bits - so this is big endian.
Actually we uses it as a string and as you can check in generated uuids 
its proper. As wiki says:
"Version 4 UUIDs have the form xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx 
where x is any hexadecimal digit and y is one of 8, 9, A, or B (e.g., 
f47ac10b-58cc-4372-a567-0e02b2c3d479)."

Even if this code runs on big-endian machine, version and variant are 
still set properly (most significant bits).

>> +/*
>> + * 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.
>

I also apply other comments to V3.
Thanks

-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com