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.
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.
- 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.