[U-Boot] [PATCH v4 2/6] lib: uuid: code refactor for proper maintain between uuid bin and string

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

Hello Stephen,
Thanks for review again:)

On 03/25/2014 08:12 PM, Stephen Warren wrote:
> On 03/19/2014 11:58 AM, Przemyslaw Marczak wrote:
>> Changes in lib/uuid.c to:
>> - uuid_str_to_bin()
>> - uuid_bin_to_str()
>>
>> New parameter is added to specify input/output string format in listed functions
>> This change allows easy recognize which UUID type is or should be stored in given
>> string array. Binary data of UUID and GUID is always stored in big endian, only
>> string representations are different as follows.
>>
>> String byte: 0                                  36
>> String char: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
>> string UUID:    be     be   be   be       be
>> string GUID:    le     le   le   be       be
>>
>> This patch also updates functions calls and declarations in a whole code.
>
> Ah, this patch pretty much solves all the comments I had on patch 1/6,
> so feel free to ignore those.
>
> Just a couple minor points below, but otherwise, patches 1 and 2,
> Acked-by: Stephen Warren <swarren@nvidia.com>
>
Ok, thank you.

>> diff --git a/include/uuid.h b/include/uuid.h
>
>> +typedef enum {
>> +	UUID_STR_FORMAT_STD,
>> +	UUID_STR_FORMAT_GUID
>> +} uuid_str_t;
>
> I would rename "STD" to "UUID"; after all, someone wanting to use GUIDs
> might think /that/ is the standard format:-)
>
> But this is a bit bike-sheddy/nit-picky, so if you don't want to I won't
> object.
>

Actually I think that UUID_STR_FORMAT_UUID gives no information that 
this the main format of UUID, so I prefer to leave STD.

>> diff --git a/lib/uuid.c b/lib/uuid.c
>
>
>> +void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str,
>> +		     uuid_str_t str_format)
>>   {
>> -	static const u8 le[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11,
>> -				  12, 13, 14, 15};
>> +	const u8 uuid_char_order[UUID_BIN_LEN] = {0, 1, 2, 3, 4, 5, 6, 7, 8,
>> +						  9, 10, 11, 12, 13, 14, 15};
>> +	const u8 guid_char_order[UUID_BIN_LEN] = {3, 2, 1, 0, 5, 4, 7, 6, 8,
>> +						  9, 10, 11, 12, 13, 14, 15};
>
> These are really more binary data order than char order, since each one
> of the bytes pointed at by entries in these arrays ends up being 2
> characters. s/char/bin/ in the variable names perhaps?
>

Yes, you are right. But according to the specification UUID and UUID bin 
format are always in big-endian - only bytes in some STRING blocks have 
different order. This works in two ways but to be consistent with 
specification I called this  as "uuid_char_order". And this is directly 
used by sprintf: "sprintf(uuid_str, "%02x", uuid_bin[char_order[i]]);".

>> +	const u8 *char_order;
>>   	int i;
>>
>> +	/*
>> +	 * UUID and GUID bin data - always in big endian:
>> +	 * 4B-2B-2B-2B-6B
>> +	 * be be be be be
>
> Strings don't really have an endianness, since they're already byte
> data. Rather than endianness, you really mean "normal numerical digit
> ordering". This comment also applies to the description of UUID string
> formats in patch 1/6.
>
Right but the comment above says about "bin" data (16B len).

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