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

[Przemyslaw Marczak]

Hello Stephen,

On 03/25/2014 08:28 PM, Stephen Warren wrote:
> On 03/19/2014 11:58 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
>
> Some nits in the comments below, but otherwise:
> Acked-by: Stephen Warren <swarren at nvidia.com>
>
>> diff --git a/include/uuid.h b/include/uuid.h
>
>> +/* This is structure is in big-endian */
>> +struct uuid {
>
> Not any more; with the introduction of enum uuid_str_t, some of the
> fields could be either LE or BE. I would say "See the comment near the
> top of lib/uuid.c for details of the endianness of fields in this struct".
>

No, those fields are always in big-endian. So no matter what is 
architecture endianess - this data should be stored as big endian. Only 
string representation has different character order for GUID.

>> diff --git a/lib/uuid.c b/lib/uuid.c
>
>>   /*
>>    * UUID - Universally Unique IDentifier - 128 bits unique number.
>>    *        There are 5 versions and one variant of UUID defined by RFC4122
>> - *        specification. Depends on version uuid number base on a time,
>> - *        host name, MAC address or random data.
>> + *        specification. Depends on version uuid number base on:
>
> I still have no idea what "Depends on version uuid number base on" means.
>

It means that each UUID version "result" depends on different source 
data, as listed here...

>> + *        - time, MAC address(v1),
>> + *        - user ID(v2),
>> + *        - MD5 of name or URL(v3),
>> + *        - random data(v4),
>> + *        - SHA-1 of name or URL(v5),
>> + *
>> + * This library implements UUID v4.
>
> I think that should say "gen_rand_uuid()" not "This library", since the
> source of the data in the UUID fields only matters when creating the
> UUID, not when performing str<->bin conversion.
>

Yes, right notice.

>> + *
>> + * Layout of UUID Version 4:

I should remove "Version 4" in the comment subject, because layout 
refers to all uuid versions.

>> + * 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.
>> + * source: https://www.ietf.org/rfc/rfc4122.txt
>
> gen_rand_uuid() doesn't actually honor that format; it creates pure
> random data rather than filling in any timestamps, clock sequence data, etc.
>

Actually, yes but two fields are NOT set randomly, and this is what 
comment includes:
"In this version all fields beside 4 bit version are randomly generated."
Moreover the gen_rand_uuid() respects endianess for setting bits,
and this could be checked on linux host by "uuid -d uboot_uuid_string" 
in shell.

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