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.marc...@samsung.com>
cc: Stephen Warren <swar...@nvidia.com>
cc: tr...@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.marc...@samsung.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
