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

[Przemyslaw Marczak]

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