Hello again,

On 02/28/2014 06:03 PM, Stephen Warren wrote:

On 02/28/2014 08:18 AM, Przemyslaw Marczak wrote:

...

Changes:

• randomly generate each partition uuid if undefined
• print info about generated uuid
• save environment on gpt write success
• update doc/README.gpt

...

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

...

static char extract_env(const char *str, char **env) {

• int ret = -1;

Why does the function return char not int? At least the type of ret should match the return type of the function.

Right notice, this should be fixed.

There's no need to introduce a ret variable anyway; just don't delete the return statements that are already in the function.

But I need to move "free(s)" so I can't leave current return statements. Other way I need to put "free(s)" in few places.

...

@@ -43,16 +44,23 @@ static char extract_env(const char *str, char **env) memset(s + strlen(s) - 1, '\0', 1); memmove(s, s + 2, strlen(s) - 1); e = getenv(s);

• free(s);
  

  if (e == NULL) {

• 	printf("Environmental '%s' not set\n", str);
  

• 	return -1; /* env not set */
  

• 	printf("%s unset. ", str);
  

• 	e = get_uuid_str();
  

• 	if (e) {
  

• 		printf("Setting to: %s\n", e);
  

• 		setenv(s, e);
  

And here I forget about free(e)...

Why should the environment variable be set? I rather dislike commands that randomly set environment variables as an implicit side-effect.

Actually automatically generated uuids was the main purpose of this patches. Setting each env variable in this place was the most easy way to make this without a lot of duplicated code.

Why do you treat it like a side-effect? If user wants have own generated uuids - then he can manually set env variables like "uuid_gpt_disk". This actually is not changed - when uuid env is set then it will be used like in current version of code. When user can't generate uuids or just wants to have it automatically generated then my code do this job.

It'd be far better if this function simply wasn't modified, but rather the user was provided with a function to explicitly set an environment variable to a randomly generated GPT. That way the user/script would be in control. Something like:

$ gen_random_uuid env_var_name

I understand that this is very important code, but setting each val manually with random uuid actually will not change anything - user still needs to do something.

The other way is to provide a function for parse e.g $partitions but then it will be a code duplication. The main job is done by set_gpt_info() so this is why I modified this code.

...

@@ -299,8 +307,17 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; }

• if (gpt_default(blk_dev_desc, argv[4]))
  

• puts("Writing GPT: ");
  

• ret = gpt_default(blk_dev_desc, argv[4]);
  

• if (!ret) {
  

• 	puts("success!\n");
  

• 	saveenv();
  

Uggh. Definitely don't save the environment behind the user's back. There is no reason to believe that's safe. What if the user had added some temporary changes to their environment that they didn't want saved? This kind of logic belongs in scripts, not code.

The one and only reason for put saveenv() here was that if uuids are randomly generated or even just are in environment then I can be sure that next gpt write (e.g. in case of overwrite sector 0 by mistake) is using the same uuids values.

Maybe saveenv() in this place is not the best idea but in other side user actually uses this command just once.

Thank you

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.

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.marczak@samsung.com cc: Stephen Warren swarren@nvidia.com cc: trini@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?

+/*

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

Changes: - randomly generate each partition uuid if undefined - print info about generated uuid - save environment on gpt write success - update doc/README.gpt

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Acked-by: Lukasz Majewski l.majewski@samsung.com cc: Piotr Wilczek p.wilczek@samsung.com cc: Tom Rini trini@ti.com

--- Changes v2: - cmd_gpt: extract_env: change return type from char to int - add tmp array to generate uuid string - store generated uuid in env and next get it from it - don't need to alloc and maintain allcoated memory outside extract_env()

--- common/cmd_gpt.c | 33 ++++++++++++++++++++++++++------- doc/README.gpt | 1 + include/common.h | 3 ++- lib/Makefile | 1 + 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index 1f12e6d..cad402f 100644 --- a/common/cmd_gpt.c +++ b/common/cmd_gpt.c @@ -29,9 +29,11 @@ * * @return - zero on successful expand and env is set */ -static char extract_env(const char *str, char **env) +static int extract_env(const char *str, char **env) { + int ret = -1; char *e, *s; + char uuid_str[37];

if (!str || strlen(str) < 4) return -1; @@ -43,16 +45,25 @@ static char extract_env(const char *str, char **env) memset(s + strlen(s) - 1, '\0', 1); memmove(s, s + 2, strlen(s) - 1); e = getenv(s); - free(s); if (e == NULL) { - printf("Environmental '%s' not set\n", str); - return -1; /* env not set */ + printf("%s unset. ", str); + gen_rand_uuid_str(uuid_str); + setenv(s, uuid_str); + + e = getenv(s); + if (e) { + puts("Setting to random.\n"); + ret = 0; + } + } else { + ret = 0; } + *env = e; - return 0; + free(s); }

- return -1; + return ret; }

/** @@ -299,8 +310,16 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; }

- if (gpt_default(blk_dev_desc, argv[4])) + puts("Writing GPT: "); + + ret = gpt_default(blk_dev_desc, argv[4]); + if (!ret) { + puts("success!\n"); + return CMD_RET_SUCCESS; + } else { + puts("error!\n"); return CMD_RET_FAILURE; + } } else { return CMD_RET_USAGE; } diff --git a/doc/README.gpt b/doc/README.gpt index 5c133f3..afe2538 100644 --- a/doc/README.gpt +++ b/doc/README.gpt @@ -176,3 +176,4 @@ Please, pay attention at -l switch for parted. "uuid" program is recommended to generate UUID string. Moreover it can decode (-d switch) passed in UUID string. It can be used to generate partitions UUID passed to u-boot environment variables. +If each partition "uuid" no exists then it will be randomly generated. diff --git a/include/common.h b/include/common.h index 20e9ae6..665c98f 100644 --- a/include/common.h +++ b/include/common.h @@ -834,7 +834,8 @@ char * strmhz(char *buf, unsigned long hz); #if defined(CONFIG_RANDOM_MACADDR) || \ defined(CONFIG_BOOTP_RANDOM_DELAY) || \ defined(CONFIG_CMD_LINK_LOCAL) || \ - defined(CONFIG_RANDOM_UUID) + defined(CONFIG_RANDOM_UUID) || \ + defined(CONFIG_PARTITION_UUIDS) #define RAND_MAX -1U void srand(unsigned int seed); unsigned int rand(void); diff --git a/lib/Makefile b/lib/Makefile index 64a430f..e989b18 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -65,6 +65,7 @@ obj-$(CONFIG_BOOTP_PXE) += uuid.o obj-$(CONFIG_PARTITION_UUIDS) += uuid.o obj-$(CONFIG_RANDOM_UUID) += uuid.o obj-$(CONFIG_RANDOM_UUID) += rand.o +obj-$(CONFIG_PARTITION_UUIDS) += rand.o obj-$(CONFIG_RANDOM_MACADDR) += rand.o obj-$(CONFIG_BOOTP_RANDOM_DELAY) += rand.o obj-$(CONFIG_CMD_LINK_LOCAL) += rand.o

Hello, Sorry for silent, but I've had some other work. I agree with yours previous comments and those will apply to v3 but I don't agree with few comments to this patch.

On 03/10/2014 06:44 PM, Stephen Warren wrote:

On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:

...

Changes:

• randomly generate each partition uuid if undefined
• print info about generated uuid
• save environment on gpt write success
• update doc/README.gpt

...

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

...

-static char extract_env(const char *str, char **env) +static int extract_env(const char *str, char **env) {

• int ret = -1; char *e, *s;

• char uuid_str[37];

  if (!str || strlen(str) < 4) return -1;

@@ -43,16 +45,25 @@ static char extract_env(const char *str, char **env) memset(s + strlen(s) - 1, '\0', 1); memmove(s, s + 2, strlen(s) - 1); e = getenv(s);

• free(s);
  

  if (e == NULL) {

• 	printf("Environmental '%s' not set\n", str);
  

• 	return -1; /* env not set */
  

• 	printf("%s unset. ", str);
  

• 	gen_rand_uuid_str(uuid_str);
  

• 	setenv(s, uuid_str);
  

In this place ret is "-1".

...

• 	e = getenv(s);
  

• 	if (e) {
  

• 		puts("Setting to random.\n");
  

Shouldn't this be printed right after the "if (e == NULL)" check above? That's where the decision is made to generate a random UUID.

Here, "if (!e)", the code should return an error.

If (!e) then ret is still "-1". If (e) then ret = 0 and proper info is printed.

But, I still don't like changing the environment. Why can't the above few lines be:

• 	gen_rand_uuid_str(uuid_str);
  

• 	e = uuid_str;
  

Such solution needs more code rewriting and breaking some existing cmd_gpt design. "e" is used outside this function but uuid_str is local here. I don't like to make it static. Using getenv and return its pointer will work the same as previous.

Please note that variables set by user are not overwritten here so this code will only set null uuid env variables. Moreover user can see after gpt command that environment is the same with mmc part shows, I think it is useful instead of situation when uuid is set but not present in environment.

...

diff --git a/doc/README.gpt b/doc/README.gpt

...

"uuid" program is recommended to generate UUID string. Moreover it can decode (-d switch) passed in UUID string. It can be used to generate partitions UUID passed to u-boot environment variables. +If each partition "uuid" no exists then it will be randomly generated.

"If each" means "if all of them", implying that it's an all-or-nothing solution, and the random generation only happens of none of the UUIDs were supplied, not on a UUID-by-UUID basis. So, s/each/a/ or s/each/any/.

Ok :)

Thanks

On 03/13/2014 08:49 PM, Stephen Warren wrote:

On 03/13/2014 11:28 AM, Przemyslaw Marczak wrote:

...

On 03/10/2014 06:44 PM, Stephen Warren wrote:

...

On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:

...

Changes:

• randomly generate each partition uuid if undefined
• print info about generated uuid
• save environment on gpt write success
• update doc/README.gpt

...

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

...

...

...

@@ -43,16 +45,25 @@ static char extract_env(const char *str, char **env) memset(s + strlen(s) - 1, '\0', 1); memmove(s, s + 2, strlen(s) - 1); e = getenv(s);

•    free(s);
       if (e == NULL) {
  

•        printf("Environmental '%s' not set\n", str);
  

•        return -1; /* env not set */
  

•        printf("%s unset. ", str);
  

•        gen_rand_uuid_str(uuid_str);
  

•        setenv(s, uuid_str);
  

In this place ret is "-1".

...

...

•        e = getenv(s);
  

•        if (e) {
  

•            puts("Setting to random.\n");
  

Shouldn't this be printed right after the "if (e == NULL)" check above? That's where the decision is made to generate a random UUID.

Here, "if (!e)", the code should return an error.

If (!e) then ret is still "-1". If (e) then ret = 0 and proper info is printed.

But ret has nothing to do with it; there's no code in this function which prints messges based on ret.

The code should be:

e = getenv(s); if (e == NULL) { printf("Env var '%s' not set; using random UUID\n", str); // generate random UUID here if (UUID generation failed) { printf("ERROR generating random UUID\n"); return -1; }

Take a note that gen_rand_uuid_str() doesn't return any error. So this error is not true.

}

With the code in your patch, the following happens:

• Env var is set: Nothing printed.

And env var is not changed - and this is as before.

• Env var not set, but problem during UUID generation: "%s unset. " is

printed without a \n at the end. The error does get propagated back tot he caller, which might print a message with \n at the end, but you shouldn't place that kind of requirement on the caller. Rather, this function should be fully self-contained.

• Env var set, and UUID generated OK: "%s unset. Setting to random.\n"

is printed.

...

...

But, I still don't like changing the environment. Why can't the above few lines be:

•         gen_rand_uuid_str(uuid_str);
  

•        e = uuid_str;
  

Such solution needs more code rewriting and breaking some existing cmd_gpt design. "e" is used outside this function but uuid_str is local here. I don't like to make it static. Using getenv and return its pointer will work the same as previous.

Please note that variables set by user are not overwritten here so this code will only set null uuid env variables. Moreover user can see after gpt command that environment is the same with mmc part shows, I think it is useful instead of situation when uuid is set but not present in environment.

I don't think convenience of coding or the size of the patch is justification for writing values to the user's environment when they didn't ask for it. What if they run saveenv after executing this function, yet they specifically want the environment variables unset, so that a random UUID is generated each time this function/command is run?

Actually I don't understand what is the problem. What is the difference when user set manually $uuid_gpt_* or use generated by gpt command if next he write "save", in both situations variables are saved. I don't think it is a problem.

Thanks

Changes: - randomly generate partition uuid if any is undefined - print info about set/unset/generated uuid - update doc/README.gpt

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Acked-by: Lukasz Majewski l.majewski@samsung.com Cc: Piotr Wilczek p.wilczek@samsung.com Cc: Tom Rini trini@ti.com Cc: Jason Hobbs jason.hobbs@calxeda.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com

--- Changes v2: - cmd_gpt: extract_env: change return type from char to int - add tmp array to generate uuid string - store generated uuid in env and next get it from it - don't need to alloc and maintain allcoated memory outside extract_env()

Changes v3: - print info if uuid_gpt_* env is get from env/set random - change some word in README.gpt to meaningful --- common/cmd_gpt.c | 36 +++++++++++++++++++++++++++++------- doc/README.gpt | 1 + include/common.h | 3 ++- lib/Makefile | 1 + 4 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index 1f12e6d..31a3fe1 100644 --- a/common/cmd_gpt.c +++ b/common/cmd_gpt.c @@ -29,9 +29,11 @@ * * @return - zero on successful expand and env is set */ -static char extract_env(const char *str, char **env) +static int extract_env(const char *str, char **env) { + int ret = -1; char *e, *s; + char uuid_str[37];

if (!str || strlen(str) < 4) return -1; @@ -43,16 +45,28 @@ static char extract_env(const char *str, char **env) memset(s + strlen(s) - 1, '\0', 1); memmove(s, s + 2, strlen(s) - 1); e = getenv(s); - free(s); if (e == NULL) { - printf("Environmental '%s' not set\n", str); - return -1; /* env not set */ + printf("%s ", str); + gen_rand_uuid_str(uuid_str); + setenv(s, uuid_str); + + e = getenv(s); + if (e) { + puts("set to random.\n"); + ret = 0; + } else { + puts("unset - can't get random UUID.\n"); + } + } else { + printf("%s get from environment.\n", str); + ret = 0; } + *env = e; - return 0; + free(s); }

- return -1; + return ret; }

/** @@ -299,8 +313,16 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; }

- if (gpt_default(blk_dev_desc, argv[4])) + puts("Writing GPT:\n"); + + ret = gpt_default(blk_dev_desc, argv[4]); + if (!ret) { + puts("success!\n"); + return CMD_RET_SUCCESS; + } else { + puts("error!\n"); return CMD_RET_FAILURE; + } } else { return CMD_RET_USAGE; } diff --git a/doc/README.gpt b/doc/README.gpt index 5c133f3..9d0a8df 100644 --- a/doc/README.gpt +++ b/doc/README.gpt @@ -176,3 +176,4 @@ Please, pay attention at -l switch for parted. "uuid" program is recommended to generate UUID string. Moreover it can decode (-d switch) passed in UUID string. It can be used to generate partitions UUID passed to u-boot environment variables. +If any partition "uuid" no exists then it will be randomly generated. diff --git a/include/common.h b/include/common.h index 20e9ae6..665c98f 100644 --- a/include/common.h +++ b/include/common.h @@ -834,7 +834,8 @@ char * strmhz(char *buf, unsigned long hz); #if defined(CONFIG_RANDOM_MACADDR) || \ defined(CONFIG_BOOTP_RANDOM_DELAY) || \ defined(CONFIG_CMD_LINK_LOCAL) || \ - defined(CONFIG_RANDOM_UUID) + defined(CONFIG_RANDOM_UUID) || \ + defined(CONFIG_PARTITION_UUIDS) #define RAND_MAX -1U void srand(unsigned int seed); unsigned int rand(void); diff --git a/lib/Makefile b/lib/Makefile index fc7a24a..f4c06c6 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_PARTITION_UUIDS) += rand.o obj-y += vsprintf.o obj-$(CONFIG_RANDOM_UUID) += uuid.o obj-$(CONFIG_RANDOM_UUID) += rand.o +obj-$(CONFIG_PARTITION_UUIDS) += rand.o obj-$(CONFIG_RANDOM_MACADDR) += rand.o obj-$(CONFIG_BOOTP_RANDOM_DELAY) += rand.o obj-$(CONFIG_CMD_LINK_LOCAL) += rand.o

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.

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com

--- Changes v4: (new commit) - change simple error checking to uuid_str_valid() in uuid_str_to_bin() - add new parameter to define UUID string format for UUID or GUID which diffe in endianness of first three string blocks. - update functions calls and declarations - add uuid.h header - remove functions declarations from common.h --- disk/part_efi.c | 17 ++++++------ include/common.h | 4 +-- include/uuid.h | 21 ++++++++++++++ lib/uuid.c | 85 ++++++++++++++++++++++++++++++++++++++++---------------- 4 files changed, 92 insertions(+), 35 deletions(-) create mode 100644 include/uuid.h

diff --git a/disk/part_efi.c b/disk/part_efi.c index a280ab5..216a292 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -100,8 +100,8 @@ void print_part_efi(block_dev_desc_t * dev_desc)

printf("Part\tStart LBA\tEnd LBA\t\tName\n"); printf("\tAttributes\n"); - printf("\tType UUID\n"); - printf("\tPartition UUID\n"); + printf("\tType GUID\n"); + printf("\tPartition GUID\n");

for (i = 0; i < le32_to_cpu(gpt_head->num_partition_entries); i++) { /* Stop at the first non valid PTE */ @@ -114,11 +114,11 @@ void print_part_efi(block_dev_desc_t * dev_desc) print_efiname(&gpt_pte[i])); printf("\tattrs:\t0x%016llx\n", gpt_pte[i].attributes.raw); uuid_bin = (unsigned char *)gpt_pte[i].partition_type_guid.b; - uuid_bin_to_str(uuid_bin, uuid); + uuid_bin_to_str(uuid_bin, uuid, UUID_STR_FORMAT_GUID); printf("\ttype:\t%s\n", uuid); uuid_bin = (unsigned char *)gpt_pte[i].unique_partition_guid.b; - uuid_bin_to_str(uuid_bin, uuid); - printf("\tuuid:\t%s\n", uuid); + uuid_bin_to_str(uuid_bin, uuid, UUID_STR_FORMAT_GUID); + printf("\tguid:\t%s\n", uuid); }

/* Remember to free pte */ @@ -165,7 +165,8 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part, sprintf((char *)info->type, "U-Boot"); info->bootable = is_bootable(&gpt_pte[part - 1]); #ifdef CONFIG_PARTITION_UUIDS - uuid_bin_to_str(gpt_pte[part - 1].unique_partition_guid.b, info->uuid); + uuid_bin_to_str(gpt_pte[part - 1].unique_partition_guid.b, info->uuid, + UUID_STR_FORMAT_GUID); #endif

debug("%s: start 0x" LBAF ", size 0x" LBAF ", name %s", __func__, @@ -323,7 +324,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, str_uuid = partitions[i].uuid; bin_uuid = gpt_e[i].unique_partition_guid.b;

- if (uuid_str_to_bin(str_uuid, bin_uuid)) { + if (uuid_str_to_bin(str_uuid, bin_uuid, UUID_STR_FORMAT_STD)) { printf("Partition no. %d: invalid guid: %s\n", i, str_uuid); return -1; @@ -370,7 +371,7 @@ int gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header *gpt_h, gpt_h->header_crc32 = 0; gpt_h->partition_entry_array_crc32 = 0;

- if (uuid_str_to_bin(str_guid, gpt_h->disk_guid.b)) + if (uuid_str_to_bin(str_guid, gpt_h->disk_guid.b, UUID_STR_FORMAT_GUID)) return -1;

return 0; diff --git a/include/common.h b/include/common.h index b1d1068..658c326 100644 --- a/include/common.h +++ b/include/common.h @@ -816,9 +816,7 @@ void udelay (unsigned long); void mdelay(unsigned long);

/* lib/uuid.c */ -void uuid_bin_to_str(unsigned char *uuid, char *str); -int uuid_str_to_bin(char *uuid, unsigned char *out); -int uuid_str_valid(const char *uuid); +#include <uuid.h>

/* lib/vsprintf.c */ #include <vsprintf.h> diff --git a/include/uuid.h b/include/uuid.h new file mode 100644 index 0000000..0d16038 --- /dev/null +++ b/include/uuid.h @@ -0,0 +1,21 @@ +/* + * Copyright (C) 2014 Samsung Electronics + * Przemyslaw Marczak p.marczak@samsung.com + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#ifndef __UUID_H__ +#define __UUID_H__ + +typedef enum { + UUID_STR_FORMAT_STD, + UUID_STR_FORMAT_GUID +} uuid_str_t; + +#define UUID_STR_LEN 36 +#define UUID_BIN_LEN 16 + +int uuid_str_valid(const char *uuid); +int uuid_str_to_bin(char *uuid_str, unsigned char *uuid_bin, uuid_str_t format); +void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, uuid_str_t format); +#endif diff --git a/lib/uuid.c b/lib/uuid.c index 03c5b97..75a5608 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -7,8 +7,9 @@ #include <linux/ctype.h> #include <errno.h> #include <common.h> - -#define UUID_STR_LEN 36 +#include <asm/io.h> +#include <part_efi.h> +#include <malloc.h>

/* * UUID - Universally Unique IDentifier - 128 bits unique number. @@ -40,7 +41,6 @@ * * GUID is used e.g. in GPT (GUID Partition Table) as a partiions unique numbers. */ - int uuid_str_valid(const char *uuid) { int i, valid; @@ -59,57 +59,94 @@ int uuid_str_valid(const char *uuid) } }

- if (i != 36 || !valid) + if (i != UUID_STR_LEN || !valid) return 0;

return 1; }

-int uuid_str_to_bin(char *uuid, unsigned char *out) +/* + * uuid_str_to_bin() - convert string UUID or GUID to big endian binary data. + * + * @param uuid_str - pointer to UUID or GUID string [37B] + * @param uuid_bin - pointer to allocated array for big endian output [16B] + * @str_format - UUID string format: 0 - UUID; 1 - GUID + */ +int uuid_str_to_bin(char *uuid_str, unsigned char *uuid_bin, + uuid_str_t str_format) { uint16_t tmp16; uint32_t tmp32; uint64_t tmp64;

- if (!uuid || !out) + if (!uuid_str_valid(uuid_str)) return -EINVAL;

- if (strlen(uuid) != UUID_STR_LEN) - return -EINVAL; + if (str_format == UUID_STR_FORMAT_STD) { + tmp32 = cpu_to_be32(simple_strtoul(uuid_str, NULL, 16)); + memcpy(uuid_bin, &tmp32, 4);

- tmp32 = cpu_to_le32(simple_strtoul(uuid, NULL, 16)); - memcpy(out, &tmp32, 4); + tmp16 = cpu_to_be16(simple_strtoul(uuid_str + 9, NULL, 16)); + memcpy(uuid_bin + 4, &tmp16, 2);

- tmp16 = cpu_to_le16(simple_strtoul(uuid + 9, NULL, 16)); - memcpy(out + 4, &tmp16, 2); + tmp16 = cpu_to_be16(simple_strtoul(uuid_str + 14, NULL, 16)); + memcpy(uuid_bin + 6, &tmp16, 2); + } else { + tmp32 = cpu_to_le32(simple_strtoul(uuid_str, NULL, 16)); + memcpy(uuid_bin, &tmp32, 4);

- tmp16 = cpu_to_le16(simple_strtoul(uuid + 14, NULL, 16)); - memcpy(out + 6, &tmp16, 2); + tmp16 = cpu_to_le16(simple_strtoul(uuid_str + 9, NULL, 16)); + memcpy(uuid_bin + 4, &tmp16, 2);

- tmp16 = cpu_to_be16(simple_strtoul(uuid + 19, NULL, 16)); - memcpy(out + 8, &tmp16, 2); + tmp16 = cpu_to_le16(simple_strtoul(uuid_str + 14, NULL, 16)); + memcpy(uuid_bin + 6, &tmp16, 2); + } + + tmp16 = cpu_to_be16(simple_strtoul(uuid_str + 19, NULL, 16)); + memcpy(uuid_bin + 8, &tmp16, 2);

- tmp64 = cpu_to_be64(simple_strtoull(uuid + 24, NULL, 16)); - memcpy(out + 10, (char *)&tmp64 + 2, 6); + tmp64 = cpu_to_be64(simple_strtoull(uuid_str + 24, NULL, 16)); + memcpy(uuid_bin + 10, (char *)&tmp64 + 2, 6);

return 0; }

-void uuid_bin_to_str(unsigned char *uuid, char *str) +/* + * uuid_bin_to_str() - convert big endian binary data to string UUID or GUID. + * + * @param uuid_bin - pointer to binary data of UUID (big endian) [16B] + * @param uuid_str - pointer to allocated array for output string [37B] + * @str_format - UUID string format: 0 - UUID; 1 - GUID + */ +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}; + 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 + */ + if (str_format == UUID_STR_FORMAT_STD) + char_order = uuid_char_order; + else + char_order = guid_char_order; + for (i = 0; i < 16; i++) { - sprintf(str, "%02x", uuid[le[i]]); - str += 2; + sprintf(uuid_str, "%02x", uuid_bin[char_order[i]]); + uuid_str += 2; switch (i) { case 3: case 5: case 7: case 9: - *str++ = '-'; + *uuid_str++ = '-'; break; } }

Those commands basis on implementation of random UUID generator version 4 which is described in RFC4122. The same algorithm is used for generation both ids but string representation is different as below.

char: 0 9 14 19 24 36 xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx UUID: be be be be be GUID: le le le be be

Commands usage: - uuid <varname(optional)> - guid <varname(optional)>

The result is saved in environment as a "varname" variable if argument is given, if not then its printed.

New config: - CONFIG_CMD_UUID

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com

--- Changes v4: - new commit --- README | 2 +- include/common.h | 4 +++- lib/Makefile | 2 ++ lib/uuid.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/README b/README index 216f0c7..f69b94e 100644 --- a/README +++ b/README @@ -1012,7 +1012,7 @@ The following options need to be configured: CONFIG_CMD_CDP * Cisco Discover Protocol support CONFIG_CMD_MFSL * Microblaze FSL support CONFIG_CMD_XIMG Load part of Multi Image - + CONFIG_CMD_UUID * Generate random UUID or GUID string

EXAMPLE: If you want all functions except of network support you can write: diff --git a/include/common.h b/include/common.h index deb08f2..de748f1 100644 --- a/include/common.h +++ b/include/common.h @@ -831,7 +831,9 @@ char * strmhz(char *buf, unsigned long hz); #if defined(CONFIG_RANDOM_MACADDR) || \ defined(CONFIG_BOOTP_RANDOM_DELAY) || \ defined(CONFIG_CMD_LINK_LOCAL) || \ - defined(CONFIG_RANDOM_UUID) + defined(CONFIG_RANDOM_UUID) || \ + defined(CONFIG_CMD_UUID) + #define RAND_MAX -1U void srand(unsigned int seed); unsigned int rand(void); diff --git a/lib/Makefile b/lib/Makefile index fd75e80..b85c825 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -65,6 +65,8 @@ obj-$(CONFIG_PARTITION_UUIDS) += uuid.o obj-y += vsprintf.o obj-$(CONFIG_RANDOM_UUID) += uuid.o obj-$(CONFIG_RANDOM_UUID) += rand.o +obj-$(CONFIG_CMD_UUID) += uuid.o +obj-$(CONFIG_CMD_UUID) += rand.o obj-$(CONFIG_RANDOM_MACADDR) += rand.o obj-$(CONFIG_BOOTP_RANDOM_DELAY) += rand.o obj-$(CONFIG_CMD_LINK_LOCAL) += rand.o diff --git a/lib/uuid.c b/lib/uuid.c index d3ba60e..0112b03 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -4,6 +4,7 @@ * SPDX-License-Identifier: GPL-2.0+ */

+#include <common.h> #include <linux/ctype.h> #include <errno.h> #include <common.h> @@ -172,7 +173,7 @@ void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, * * @param uuid_bin - pointer to allocated array [16B]. Output is in big endian. */ -#ifdef CONFIG_RANDOM_UUID +#if defined(CONFIG_RANDOM_UUID) || defined(CONFIG_CMD_UUID) void gen_rand_uuid(unsigned char *uuid_bin) { struct uuid uuid; @@ -211,4 +212,45 @@ void gen_rand_uuid_str(char *uuid_str, uuid_str_t str_format) /* Convert UUID bin to UUID or GUID formated STRING */ uuid_bin_to_str(uuid_bin, uuid_str, str_format); } + +#ifdef CONFIG_CMD_UUID +int do_uuid(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + char uuid[UUID_STR_LEN + 1]; + uuid_str_t str_format; + + if (!strcmp(argv[0], "uuid")) + str_format = UUID_STR_FORMAT_STD; + else + str_format = UUID_STR_FORMAT_GUID; + + if (argc == 1) { + gen_rand_uuid_str(uuid, str_format); + printf("%s\n", uuid); + } else if (argc == 2) { + gen_rand_uuid_str(uuid, str_format); + setenv(argv[1], uuid); + } else { + return CMD_RET_USAGE; + } + + return CMD_RET_SUCCESS; +} + +U_BOOT_CMD(uuid, CONFIG_SYS_MAXARGS, 1, do_uuid, + "UUID - generate Universally Unique Identifier version 4", + "<varname(optional)>\n" + "Argument:\n" + "varname: for set result in a environment variable\n" + "e.g. uuid uuid_env" +); + +U_BOOT_CMD(guid, CONFIG_SYS_MAXARGS, 1, do_uuid, + "GUID - generate Globally Unique Identifier based on UUID version 4", + "<varname(optional)>\n" + "Argument:\n" + "varname: for set result in a environment variable\n" + "e.g. guid guid_env" +); +#endif #endif

On 03/19/2014 11:58 AM, Przemyslaw Marczak wrote:

Changes:

• randomly generate partition uuid if any is undefined and CONFIG_RAND_UUID is defined
• print debug info about set/unset/generated uuid
• update doc/README.gpt

Update existing code to the new library functions.

The changelog should be below the --- line, and a patch description should exist.

Assuming the comments below are fixed, Acked-by: Stephen Warren swarren@nvidia.com

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

-static char extract_env(const char *str, char **env) +static int extract_env(const char *str, char **env) {

• int ret = -1; char *e, *s;

+#ifdef CONFIG_RANDOM_UUID

• char uuid_str[UUID_STR_LEN + 1];

+#endif if (!str || strlen(str) < 4)

The blank line needs to be after the #endif not before the #ifdef, so the variable declarations are separate from the code.

return -1;

• if ((strncmp(str, "${", 2) == 0) && (str[strlen(str) - 1] == '}')) {

• s = strdup(str);
  

• if (s == NULL)
  

• 	return -1;
  

• memset(s + strlen(s) - 1, '\0', 1);
  

• memmove(s, s + 2, strlen(s) - 1);
  

• if (!((strncmp(str, "${", 2) == 0) && (str[strlen(str) - 1] == '}')))

• return -1;
  

Since you're inverting that test, you need to change && to || too.

diff --git a/doc/README.gpt b/doc/README.gpt index 5c133f3..51515c8 100644 --- a/doc/README.gpt +++ b/doc/README.gpt @@ -101,7 +101,7 @@ Offset Size Description 40 8 B First usable LBA for partitions (primary partition table last LBA + 1) 48 8 B Last usable LBA (secondary partition table first LBA - 1) -56 16 B Disk GUID (also referred as UUID on UNIXes) +56 16 B Disk GUID (also referred as UUID on UNIXes) in big endian

According to your earlier comment, GUIDs have a mix of LE and BE fields, so I would simply drop this change and the similar change below. Let wikipedia or the comment you added near to top of lib/uuid.c specify the details.

@@ -160,6 +160,9 @@ To restore GUID partition table one needs to: Fields 'name', 'size' and 'uuid' are mandatory for every partition. The field 'start' is optional.

• option: CONFIG_RANDOM_UUID
• If any partition "uuid" no exists then it is randomly generated.

s/"uuid"/UUID/

On 03/26/2014 06:01 AM, Przemyslaw Marczak wrote:

On 03/25/2014 08:51 PM, Stephen Warren wrote:

...

On 03/19/2014 11:58 AM, Przemyslaw Marczak wrote:

...

Changes:

• randomly generate partition uuid if any is undefined and

CONFIG_RAND_UUID is defined

• print debug info about set/unset/generated uuid
• update doc/README.gpt

Update existing code to the new library functions.

...

...

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

• if ((strncmp(str, "${", 2) == 0) && (str[strlen(str) - 1] == '}')) {

•    s = strdup(str);
  

•    if (s == NULL)
  

•        return -1;
  

•    memset(s + strlen(s) - 1, '\0', 1);
  

•    memmove(s, s + 2, strlen(s) - 1);
  

• if (!((strncmp(str, "${", 2) == 0) && (str[strlen(str) - 1] == '}')))

•    return -1;
  

Since you're inverting that test, you need to change && to || too.

No, because the invertion refers to the result of "if" - not one of conditions. !(cond1 && cond2) is the same as: (!cond1 || !cond2) so this change is ok.

Ah yes, right.

...

...

diff --git a/doc/README.gpt b/doc/README.gpt index 5c133f3..51515c8 100644 --- a/doc/README.gpt +++ b/doc/README.gpt @@ -101,7 +101,7 @@ Offset Size Description 40 8 B First usable LBA for partitions (primary partition table last LBA + 1) 48 8 B Last usable LBA (secondary partition table first LBA - 1) -56 16 B Disk GUID (also referred as UUID on UNIXes) +56 16 B Disk GUID (also referred as UUID on UNIXes) in big endian

According to your earlier comment, GUIDs have a mix of LE and BE fields, so I would simply drop this change and the similar change below. Let wikipedia or the comment you added near to top of lib/uuid.c specify the details.

Actually I think that this is an important info here. The information about endianness is also placed in few places in lib/uuid.c

Why isn't the endianness of all the fields in this structure defined in this comment then?

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com Cc: Piotr Wilczek p.wilczek@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com --- include/configs/trats.h | 1 + include/configs/trats2.h | 1 + 2 files changed, 2 insertions(+)

diff --git a/include/configs/trats.h b/include/configs/trats.h index 7cea259..30a978c 100644 --- a/include/configs/trats.h +++ b/include/configs/trats.h @@ -276,6 +276,7 @@ /* GPT */ #define CONFIG_EFI_PARTITION #define CONFIG_PARTITION_UUIDS +#define CONFIG_RANDOM_UUID

#define CONFIG_SYS_INIT_SP_ADDR (CONFIG_SYS_LOAD_ADDR - GENERATED_GBL_DATA_SIZE) #define CONFIG_SYS_CACHELINE_SIZE 32 diff --git a/include/configs/trats2.h b/include/configs/trats2.h index 6d389df..df98b70 100644 --- a/include/configs/trats2.h +++ b/include/configs/trats2.h @@ -283,6 +283,7 @@ #define CONFIG_ENV_OFFSET ((32 - 4) << 10) /* 32KiB - 4KiB */ #define CONFIG_EFI_PARTITION #define CONFIG_PARTITION_UUIDS +#define CONFIG_RANDOM_UUID

#define CONFIG_BOARD_EARLY_INIT_F

This commit introduces cleanup for uuid library. Changes: - move uuid<->string conversion functions into lib/uuid.c so they can be used by code outside part_efi.c. - rename uuid_string() to uuid_bin_to_str() for consistency with existing uuid_str_to_bin() - add an error return code to uuid_str_to_bin() - update existing code to the new library functions.

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com

--- Changes v2: - This commit is new after separate: [PATCH 1/2] lib: uuid: add function to generate UUID version 4 - it introduces small refactor of common lib uuid functions

Changes v3: - reword commit message - add UUID_STR_LEN definition in lib/uuid.c - remove unused string pointer from uuid_bin_to_str()

Changes v4: - add uuid/guid description

Changes v5: - none --- disk/part_efi.c | 90 +++++++------------------------------------------------- include/common.h | 3 +- lib/Makefile | 1 + lib/uuid.c | 61 +++++++++++++++++++++++++++++++++----- 4 files changed, 68 insertions(+), 87 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c index 733d5bd..a280ab5 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -63,26 +63,6 @@ static char *print_efiname(gpt_entry *pte) return name; }

-static void uuid_string(unsigned char *uuid, char *str) -{ - static const u8 le[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, - 12, 13, 14, 15}; - int i; - - for (i = 0; i < 16; i++) { - sprintf(str, "%02x", uuid[le[i]]); - str += 2; - switch (i) { - case 3: - case 5: - case 7: - case 9: - *str++ = '-'; - break; - } - } -} - static efi_guid_t system_guid = PARTITION_SYSTEM_GUID;

static inline int is_bootable(gpt_entry *p) @@ -103,6 +83,7 @@ void print_part_efi(block_dev_desc_t * dev_desc) gpt_entry *gpt_pte = NULL; int i = 0; char uuid[37]; + unsigned char *uuid_bin;

if (!dev_desc) { printf("%s: Invalid Argument(s)\n", __func__); @@ -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); printf("\ttype:\t%s\n", uuid); - uuid_string(gpt_pte[i].unique_partition_guid.b, uuid); + uuid_bin = (unsigned char *)gpt_pte[i].unique_partition_guid.b; + uuid_bin_to_str(uuid_bin, uuid); printf("\tuuid:\t%s\n", uuid); }

@@ -182,7 +165,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part, sprintf((char *)info->type, "U-Boot"); info->bootable = is_bootable(&gpt_pte[part - 1]); #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

debug("%s: start 0x" LBAF ", size 0x" LBAF ", name %s", __func__, @@ -237,60 +220,6 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc) return 0; }

-/** - * string_uuid(); Convert UUID stored as string to bytes - * - * @param uuid - UUID represented as string - * @param dst - GUID buffer - * - * @return return 0 on successful conversion - */ -static int string_uuid(char *uuid, u8 *dst) -{ - efi_guid_t guid; - u16 b, c, d; - u64 e; - u32 a; - u8 *p; - u8 i; - - const u8 uuid_str_len = 36; - - /* The UUID is written in text: */ - /* 1 9 14 19 24 */ - /* xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx */ - - debug("%s: uuid: %s\n", __func__, uuid); - - if (strlen(uuid) != uuid_str_len) - return -1; - - for (i = 0; i < uuid_str_len; i++) { - if ((i == 8) || (i == 13) || (i == 18) || (i == 23)) { - if (uuid[i] != '-') - return -1; - } else { - if (!isxdigit(uuid[i])) - return -1; - } - } - - a = (u32)simple_strtoul(uuid, NULL, 16); - b = (u16)simple_strtoul(uuid + 9, NULL, 16); - c = (u16)simple_strtoul(uuid + 14, NULL, 16); - d = (u16)simple_strtoul(uuid + 19, NULL, 16); - e = (u64)simple_strtoull(uuid + 24, NULL, 16); - - p = (u8 *) &e; - guid = EFI_GUID(a, b, c, d >> 8, d & 0xFF, - *(p + 5), *(p + 4), *(p + 3), - *(p + 2), *(p + 1) , *p); - - memcpy(dst, guid.b, sizeof(efi_guid_t)); - - return 0; -} - int write_gpt_table(block_dev_desc_t *dev_desc, gpt_header *gpt_h, gpt_entry *gpt_e) { @@ -358,6 +287,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, size_t efiname_len, dosname_len; #ifdef CONFIG_PARTITION_UUIDS char *str_uuid; + unsigned char *bin_uuid; #endif

for (i = 0; i < parts; i++) { @@ -391,7 +321,9 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,

#ifdef CONFIG_PARTITION_UUIDS str_uuid = partitions[i].uuid; - if (string_uuid(str_uuid, gpt_e[i].unique_partition_guid.b)) { + bin_uuid = gpt_e[i].unique_partition_guid.b; + + if (uuid_str_to_bin(str_uuid, bin_uuid)) { printf("Partition no. %d: invalid guid: %s\n", i, str_uuid); return -1; @@ -438,7 +370,7 @@ int gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header *gpt_h, gpt_h->header_crc32 = 0; gpt_h->partition_entry_array_crc32 = 0;

- if (string_uuid(str_guid, gpt_h->disk_guid.b)) + if (uuid_str_to_bin(str_guid, gpt_h->disk_guid.b)) return -1;

return 0; diff --git a/include/common.h b/include/common.h index 072a1e1..c48c696 100644 --- a/include/common.h +++ b/include/common.h @@ -822,7 +822,8 @@ void udelay (unsigned long); void mdelay(unsigned long);

/* lib/uuid.c */ -void uuid_str_to_bin(const char *uuid, unsigned char *out); +void uuid_bin_to_str(unsigned char *uuid, char *str); +int uuid_str_to_bin(char *uuid, unsigned char *out); int uuid_str_valid(const char *uuid);

/* lib/vsprintf.c */ diff --git a/lib/Makefile b/lib/Makefile index ae80865..d7ff7ca 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -61,6 +61,7 @@ obj-y += string.o obj-y += time.o obj-$(CONFIG_TRACE) += trace.o obj-$(CONFIG_BOOTP_PXE) += uuid.o +obj-$(CONFIG_PARTITION_UUIDS) += uuid.o obj-y += vsprintf.o obj-$(CONFIG_LIB_RAND) += rand.o

diff --git a/lib/uuid.c b/lib/uuid.c index c48bf38..3af3a7d 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -5,18 +5,40 @@ */

#include <linux/ctype.h> -#include "common.h" +#include <errno.h> +#include <common.h> + +#define UUID_STR_LEN 36

/* - * This is what a UUID string looks like. + * 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. + * + * UUID binary format (16 bytes): + * + * 4B-2B-2B-2B-6B (big endian - network byte order) + * + * UUID string is 36 length of characters (36 bytes): + * + * 0 9 14 19 24 + * xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx + * be be be be be + * + * where x is a hexadecimal character. Fields are separated by '-'s. + * When converting to a binary UUID, le means the field should be converted + * to little endian and be means it should be converted to big endian. * - * x is a hexadecimal character. fields are separated by '-'s. When converting - * to a binary UUID, le means the field should be converted to little endian, - * and be means it should be converted to big endian. + * UUID is also used as GUID (Globally Unique Identifier) with the same binary + * format but it differs in string format like below. * + * GUID: * 0 9 14 19 24 * xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx * le le le be be + * + * GUID is used e.g. in GPT (GUID Partition Table) as a partiions unique id. */

int uuid_str_valid(const char *uuid) @@ -43,14 +65,17 @@ int uuid_str_valid(const char *uuid) return 1; }

-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 (strlen(uuid) != UUID_STR_LEN) + return -EINVAL;

tmp32 = cpu_to_le32(simple_strtoul(uuid, NULL, 16)); memcpy(out, &tmp32, 4); @@ -66,4 +91,26 @@ void uuid_str_to_bin(const char *uuid, unsigned char *out)

tmp64 = cpu_to_be64(simple_strtoull(uuid + 24, NULL, 16)); memcpy(out + 10, (char *)&tmp64 + 2, 6); + + return 0; +} + +void uuid_bin_to_str(unsigned char *uuid, char *str) +{ + static const u8 le[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, + 12, 13, 14, 15}; + int i; + + for (i = 0; i < 16; i++) { + sprintf(str, "%02x", uuid[le[i]]); + str += 2; + switch (i) { + case 3: + case 5: + case 7: + case 9: + *str++ = '-'; + break; + } + } }

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: - new configs: - CONFIG_LIB_UUID for compile lib/uuid.c - CONFIG_RANDOM_UUID for functions gen_rand_uuid() and gen_rand_uuid_str() - add configs dependency to include/config_fallbacks.h for lib uuid.

lib/uuid.c: - add gen_rand_uuid() - this function writes 16 bytes len binary representation of UUID v4 to the memory at given address.

- add gen_rand_uuid_str() - this function writes 37 bytes len hexadecimal ASCII string representation of UUID v4 to the memory at given address.

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com

--- Changes v2: - put uuid generation changes in a separate commit - get_uuid_str() - change name to gen_rand_uuid_str() - add new function: gen_rand_uuid() - remove unnecessary '\0' at the end of uuid string - drop unnecessary error checking - functions now takes pointers to allocated memory instead of alloc it itself - add new config option: CONFIG_RANDOM_UUID

Changes v3: - remove unused UUID_STR_BYTE_LEN - reword comments - remove null pointer checking from gen_rand_uuid() and gen_rand_uuid_str() - remove unneeded memset from gen_rand_uuid() - undo moving vsprintf.o object in lib/Makefile - add attribute "packed" to the uuid structure - gen_rand_uuid(): add endian functions for modify uuid data - gen_rand_uuid(): use memcpy() to store uuid data into given buffer for avoi unaligned access issues - change uuid version and variant masks to proper for use with clrsetbits_* - add #ifdef CONFIG_RANDOM_UUID to random uuid code for avoid warnings

Changes v4: - add new parameter to define UUID string format for UUID or GUID which differs in endianness of first three string blocks. - add uuid structure and version 4 data to uuid header file - lib/Makefile: add CONFIG_RAND_UUID dependency to rand.o and uuid.o

Changes v5: - reword some comments - introduce config CMD_LIB_UUID, cleanup config dependency - remove use of typedef uuid_str_t --- include/config_fallbacks.h | 13 +++++++++ include/uuid.h | 22 +++++++++++++++- lib/Makefile | 3 +-- lib/uuid.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h index d8339b2..f31a2f5 100644 --- a/include/config_fallbacks.h +++ b/include/config_fallbacks.h @@ -55,6 +55,19 @@ #define HAVE_BLOCK_DEVICE #endif

+#if (defined(CONFIG_PARTITION_UUIDS) || \ + defined(CONFIG_RANDOM_UUID) || \ + defined(CONFIG_BOOTP_PXE)) && \ + !defined(CONFIG_LIB_UUID) +#define CONFIG_LIB_UUID +#endif + +#if defined(CONFIG_RANDOM_UUID) && \ + !defined(CONFIG_LIB_RAND) && \ + !defined(CONFIG_LIB_HW_RAND) +#define CONFIG_LIB_RAND +#endif + #ifndef CONFIG_SYS_PROMPT #define CONFIG_SYS_PROMPT "=> " #endif diff --git a/include/uuid.h b/include/uuid.h index e8feeed..93027c1 100644 --- a/include/uuid.h +++ b/include/uuid.h @@ -7,15 +7,35 @@ #ifndef __UUID_H__ #define __UUID_H__

+/* This is structure is in big-endian */ +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]; +} __packed; + enum { UUID_STR_FORMAT_STD, UUID_STR_FORMAT_GUID };

#define UUID_STR_LEN 36 -#define UUID_BIN_LEN 16 +#define UUID_BIN_LEN sizeof(struct uuid) + +#define UUID_VERSION_MASK 0xf000 +#define UUID_VERSION_SHIFT 12 +#define UUID_VERSION 0x4 + +#define UUID_VARIANT_MASK 0xc0 +#define UUID_VARIANT_SHIFT 7 +#define UUID_VARIANT 0x1

int uuid_str_valid(const char *uuid); int uuid_str_to_bin(char *uuid_str, unsigned char *uuid_bin, int str_format); void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, int str_format); +void gen_rand_uuid(unsigned char *uuid_bin); +void gen_rand_uuid_str(char *uuid_str, int str_format); #endif diff --git a/lib/Makefile b/lib/Makefile index d7ff7ca..27e4f78 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -60,8 +60,7 @@ obj-$(CONFIG_REGEX) += slre.o obj-y += string.o obj-y += time.o obj-$(CONFIG_TRACE) += trace.o -obj-$(CONFIG_BOOTP_PXE) += uuid.o -obj-$(CONFIG_PARTITION_UUIDS) += uuid.o +obj-$(CONFIG_LIB_UUID) += uuid.o obj-y += vsprintf.o obj-$(CONFIG_LIB_RAND) += rand.o

diff --git a/lib/uuid.c b/lib/uuid.c index 8db865f..44d0c93 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -14,8 +14,22 @@ /* * 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. A UUID contains a set of fields. The set varies + * depending on the version of the UUID, as shown below: + * - time, MAC address(v1), + * - user ID(v2), + * - MD5 of name or URL(v3), + * - random data(v4), + * - SHA-1 of name or URL(v5), + * + * Layout of UUID: + * 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 + * + * source: https://www.ietf.org/rfc/rfc4122.txt * * UUID binary format (16 bytes): * @@ -149,3 +163,51 @@ void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, int str_format) } } } + +/* + * gen_rand_uuid() - this function generates a random binary UUID version 4. + * In this version all fields beside 4 bits of version and + * 2 bits of variant are randomly generated. + * + * @param uuid_bin - pointer to allocated array [16B]. Output is in big endian. +*/ +#ifdef CONFIG_RANDOM_UUID +void gen_rand_uuid(unsigned char *uuid_bin) +{ + struct uuid uuid; + unsigned int *ptr = (unsigned int *)&uuid; + int i; + + /* Set all fields randomly */ + for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++) + *(ptr + i) = cpu_to_be32(rand()); + + clrsetbits_be16(&uuid.time_hi_and_version, + UUID_VERSION_MASK, + UUID_VERSION << UUID_VERSION_SHIFT); + + clrsetbits_8(&uuid.clock_seq_hi_and_reserved, + UUID_VARIANT_MASK, + UUID_VARIANT << UUID_VARIANT_SHIFT); + + memcpy(uuid_bin, &uuid, sizeof(struct uuid)); +} + +/* + * gen_rand_uuid_str() - this function generates UUID v4 (random) in two string + * formats UUID or GUID. + * + * @param uuid_str - pointer to allocated array [37B]. + * @param - uuid output type: UUID - 0, GUID - 1 + */ +void gen_rand_uuid_str(char *uuid_str, int str_format) +{ + unsigned char uuid_bin[UUID_BIN_LEN]; + + /* Generate UUID (big endian) */ + gen_rand_uuid(uuid_bin); + + /* Convert UUID bin to UUID or GUID formated STRING */ + uuid_bin_to_str(uuid_bin, uuid_str, str_format); +} +#endif

Changes: - randomly generate partition uuid if any is undefined and CONFIG_RAND_UUID is defined - print debug info about set/unset/generated uuid - update doc/README.gpt

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Acked-by: Lukasz Majewski l.majewski@samsung.com Cc: Piotr Wilczek p.wilczek@samsung.com Cc: Tom Rini trini@ti.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com

--- Changes v2: - cmd_gpt: extract_env: change return type from char to int - add tmp array to generate uuid string - store generated uuid in env and next get it from it - don't need to alloc and maintain allcoated memory outside extract_env()

Changes v3: - print info if uuid_gpt_* env is get from env/set random - change some word in README.gpt to meaningful

Changes v4: - change printf/puts to debug - reduce indentation level in extract_env() - generate rand uuid if CONFIG_RAND_UUID is defined

Changes v5: - cosmetic changes only --- common/cmd_gpt.c | 61 ++++++++++++++++++++++++++++++++++++++++++-------------- doc/README.gpt | 22 +++++++++++++++----- lib/uuid.c | 4 ++-- 3 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index 1f12e6d..e38422d 100644 --- a/common/cmd_gpt.c +++ b/common/cmd_gpt.c @@ -29,30 +29,53 @@ * * @return - zero on successful expand and env is set */ -static char extract_env(const char *str, char **env) +static int extract_env(const char *str, char **env) { + int ret = -1; char *e, *s; +#ifdef CONFIG_RANDOM_UUID + char uuid_str[UUID_STR_LEN + 1]; +#endif

if (!str || strlen(str) < 4) return -1;

- if ((strncmp(str, "${", 2) == 0) && (str[strlen(str) - 1] == '}')) { - s = strdup(str); - if (s == NULL) - return -1; - memset(s + strlen(s) - 1, '\0', 1); - memmove(s, s + 2, strlen(s) - 1); + if (!((strncmp(str, "${", 2) == 0) && (str[strlen(str) - 1] == '}'))) + return -1; + + s = strdup(str); + if (s == NULL) + return -1; + + memset(s + strlen(s) - 1, '\0', 1); + memmove(s, s + 2, strlen(s) - 1); + + e = getenv(s); + if (e == NULL) { +#ifdef CONFIG_RANDOM_UUID + debug("%s unset. ", str); + gen_rand_uuid_str(uuid_str, UUID_STR_FORMAT_STD); + setenv(s, uuid_str); + e = getenv(s); - free(s); - if (e == NULL) { - printf("Environmental '%s' not set\n", str); - return -1; /* env not set */ + if (e) { + debug("Set to random.\n"); + ret = 0; + } else { + debug("Can't get random UUID.\n"); } - *env = e; - return 0; +#else + debug("%s unset.\n", str); +#endif + } else { + debug("%s get from environment.\n", str); + ret = 0; }

- return -1; + *env = e; + free(s); + + return ret; }

/** @@ -299,8 +322,16 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; }

- if (gpt_default(blk_dev_desc, argv[4])) + puts("Writing GPT: "); + + ret = gpt_default(blk_dev_desc, argv[4]); + if (!ret) { + puts("success!\n"); + return CMD_RET_SUCCESS; + } else { + puts("error!\n"); return CMD_RET_FAILURE; + } } else { return CMD_RET_USAGE; } diff --git a/doc/README.gpt b/doc/README.gpt index 5c133f3..f822894 100644 --- a/doc/README.gpt +++ b/doc/README.gpt @@ -132,8 +132,8 @@ of the Primary. ---------------------- Offset Size Description

- 0 16 B Partition type GUID - 16 16 B Unique partition GUID + 0 16 B Partition type GUID (Big Endian) + 16 16 B Unique partition GUID in (Big Endian) 32 8 B First LBA (Little Endian) 40 8 B Last LBA (inclusive) 48 8 B Attribute flags [+] @@ -160,6 +160,9 @@ To restore GUID partition table one needs to: Fields 'name', 'size' and 'uuid' are mandatory for every partition. The field 'start' is optional.

+ option: CONFIG_RANDOM_UUID + If any partition "UUID" no exists then it is randomly generated. + 2. Define 'CONFIG_EFI_PARTITION' and 'CONFIG_CMD_GPT'

2. From u-boot prompt type: @@ -168,11 +171,20 @@ To restore GUID partition table one needs to: Useful info: ============

-Two programs, namely: 'fdisk' and 'parted' are recommended to work with GPT -recovery. Parted is able to handle GUID partitions. Unfortunately the 'fdisk' -hasn't got such ability. +Two programs, namely: 'gdisk' and 'parted' are recommended to work with GPT +recovery. Both are able to handle GUID partitions. Please, pay attention at -l switch for parted.

"uuid" program is recommended to generate UUID string. Moreover it can decode (-d switch) passed in UUID string. It can be used to generate partitions UUID passed to u-boot environment variables. +If optional CONFIG_RANDOM_UUID is defined then for any partition which environment +uuid is unset, uuid is randomly generated and stored in correspond environment +variable. + +note: +Each string block of UUID generated by program "uuid" is in big endian and it is +also stored in big endian in disk GPT. +Partitions layout can be printed by typing "mmc part". Note that each partition +GUID has different byte order than UUID generated before, this is because first +three blocks of GUID string are in Little Endian. diff --git a/lib/uuid.c b/lib/uuid.c index f32b602..f6b4423 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -251,5 +251,5 @@ U_BOOT_CMD(guid, CONFIG_SYS_MAXARGS, 1, do_uuid, "varname: for set result in a environment variable\n" "e.g. guid guid_env" ); -#endif -#endif +#endif /* CONFIG_CMD_UUID */ +#endif /* CONFIG_RANDOM_UUID || CONFIG_CMD_UUID */

On 01/04/14 23:30, Przemyslaw Marczak wrote:

This change enables automatically uuid generation by command gpt. In case of updating partitions layout user don't need to care about generate uuid manually.

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com Cc: Piotr Wilczek p.wilczek@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com

---

Changes v5:

• add commit message

---

include/configs/trats.h | 1 + include/configs/trats2.h | 1 + 2 files changed, 2 insertions(+)

diff --git a/include/configs/trats.h b/include/configs/trats.h index c00d60a..1ed4b9f 100644 --- a/include/configs/trats.h +++ b/include/configs/trats.h @@ -276,6 +276,7 @@ /* GPT */ #define CONFIG_EFI_PARTITION #define CONFIG_PARTITION_UUIDS +#define CONFIG_RANDOM_UUID

#define CONFIG_SYS_INIT_SP_ADDR (CONFIG_SYS_LOAD_ADDR - GENERATED_GBL_DATA_SIZE) #define CONFIG_SYS_CACHELINE_SIZE 32 diff --git a/include/configs/trats2.h b/include/configs/trats2.h index 59896b1..6d89ca9 100644 --- a/include/configs/trats2.h +++ b/include/configs/trats2.h @@ -283,6 +283,7 @@ #define CONFIG_ENV_OFFSET ((32 - 4) << 10) /* 32KiB - 4KiB */ #define CONFIG_EFI_PARTITION #define CONFIG_PARTITION_UUIDS +#define CONFIG_RANDOM_UUID

#define CONFIG_BOARD_EARLY_INIT_F

Acked-by: Minkyu Kang mk7.kang@samsung.com

Thanks, Minkyu Kang.

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.

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com

--- Changes v4: (new commit) - change simple error checking to uuid_str_valid() in uuid_str_to_bin() - add new parameter to define UUID string format for UUID or GUID which diffe in endianness of first three string blocks. - update functions calls and declarations - add uuid.h header - remove functions declarations from common.h

Changes v5: - update file: net/bootp.c - remove previous added uuid_str_t typedef

Changes v6: - none --- disk/part_efi.c | 17 ++++++------ include/common.h | 4 +-- include/uuid.h | 21 ++++++++++++++ lib/uuid.c | 83 ++++++++++++++++++++++++++++++++++++++++---------------- net/bootp.c | 2 +- 5 files changed, 91 insertions(+), 36 deletions(-) create mode 100644 include/uuid.h

diff --git a/disk/part_efi.c b/disk/part_efi.c index a280ab5..216a292 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -100,8 +100,8 @@ void print_part_efi(block_dev_desc_t * dev_desc)

printf("Part\tStart LBA\tEnd LBA\t\tName\n"); printf("\tAttributes\n"); - printf("\tType UUID\n"); - printf("\tPartition UUID\n"); + printf("\tType GUID\n"); + printf("\tPartition GUID\n");

for (i = 0; i < le32_to_cpu(gpt_head->num_partition_entries); i++) { /* Stop at the first non valid PTE */ @@ -114,11 +114,11 @@ void print_part_efi(block_dev_desc_t * dev_desc) print_efiname(&gpt_pte[i])); printf("\tattrs:\t0x%016llx\n", gpt_pte[i].attributes.raw); uuid_bin = (unsigned char *)gpt_pte[i].partition_type_guid.b; - uuid_bin_to_str(uuid_bin, uuid); + uuid_bin_to_str(uuid_bin, uuid, UUID_STR_FORMAT_GUID); printf("\ttype:\t%s\n", uuid); uuid_bin = (unsigned char *)gpt_pte[i].unique_partition_guid.b; - uuid_bin_to_str(uuid_bin, uuid); - printf("\tuuid:\t%s\n", uuid); + uuid_bin_to_str(uuid_bin, uuid, UUID_STR_FORMAT_GUID); + printf("\tguid:\t%s\n", uuid); }

/* Remember to free pte */ @@ -165,7 +165,8 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part, sprintf((char *)info->type, "U-Boot"); info->bootable = is_bootable(&gpt_pte[part - 1]); #ifdef CONFIG_PARTITION_UUIDS - uuid_bin_to_str(gpt_pte[part - 1].unique_partition_guid.b, info->uuid); + uuid_bin_to_str(gpt_pte[part - 1].unique_partition_guid.b, info->uuid, + UUID_STR_FORMAT_GUID); #endif

debug("%s: start 0x" LBAF ", size 0x" LBAF ", name %s", __func__, @@ -323,7 +324,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, str_uuid = partitions[i].uuid; bin_uuid = gpt_e[i].unique_partition_guid.b;

- if (uuid_str_to_bin(str_uuid, bin_uuid)) { + if (uuid_str_to_bin(str_uuid, bin_uuid, UUID_STR_FORMAT_STD)) { printf("Partition no. %d: invalid guid: %s\n", i, str_uuid); return -1; @@ -370,7 +371,7 @@ int gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header *gpt_h, gpt_h->header_crc32 = 0; gpt_h->partition_entry_array_crc32 = 0;

- if (uuid_str_to_bin(str_guid, gpt_h->disk_guid.b)) + if (uuid_str_to_bin(str_guid, gpt_h->disk_guid.b, UUID_STR_FORMAT_GUID)) return -1;

return 0; diff --git a/include/common.h b/include/common.h index c48c696..cbd3c9e 100644 --- a/include/common.h +++ b/include/common.h @@ -822,9 +822,7 @@ void udelay (unsigned long); void mdelay(unsigned long);

/* lib/uuid.c */ -void uuid_bin_to_str(unsigned char *uuid, char *str); -int uuid_str_to_bin(char *uuid, unsigned char *out); -int uuid_str_valid(const char *uuid); +#include <uuid.h>

/* lib/vsprintf.c */ #include <vsprintf.h> diff --git a/include/uuid.h b/include/uuid.h new file mode 100644 index 0000000..e8feeed --- /dev/null +++ b/include/uuid.h @@ -0,0 +1,21 @@ +/* + * Copyright (C) 2014 Samsung Electronics + * Przemyslaw Marczak p.marczak@samsung.com + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#ifndef __UUID_H__ +#define __UUID_H__ + +enum { + UUID_STR_FORMAT_STD, + UUID_STR_FORMAT_GUID +}; + +#define UUID_STR_LEN 36 +#define UUID_BIN_LEN 16 + +int uuid_str_valid(const char *uuid); +int uuid_str_to_bin(char *uuid_str, unsigned char *uuid_bin, int str_format); +void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, int str_format); +#endif diff --git a/lib/uuid.c b/lib/uuid.c index 3af3a7d..8db865f 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -7,8 +7,9 @@ #include <linux/ctype.h> #include <errno.h> #include <common.h> - -#define UUID_STR_LEN 36 +#include <asm/io.h> +#include <part_efi.h> +#include <malloc.h>

/* * UUID - Universally Unique IDentifier - 128 bits unique number. @@ -40,7 +41,6 @@ * * GUID is used e.g. in GPT (GUID Partition Table) as a partiions unique id. */ - int uuid_str_valid(const char *uuid) { int i, valid; @@ -59,57 +59,92 @@ int uuid_str_valid(const char *uuid) } }

- if (i != 36 || !valid) + if (i != UUID_STR_LEN || !valid) return 0;

return 1; }

-int uuid_str_to_bin(char *uuid, unsigned char *out) +/* + * uuid_str_to_bin() - convert string UUID or GUID to big endian binary data. + * + * @param uuid_str - pointer to UUID or GUID string [37B] + * @param uuid_bin - pointer to allocated array for big endian output [16B] + * @str_format - UUID string format: 0 - UUID; 1 - GUID + */ +int uuid_str_to_bin(char *uuid_str, unsigned char *uuid_bin, int str_format) { uint16_t tmp16; uint32_t tmp32; uint64_t tmp64;

- if (!uuid || !out) + if (!uuid_str_valid(uuid_str)) return -EINVAL;

- if (strlen(uuid) != UUID_STR_LEN) - return -EINVAL; + if (str_format == UUID_STR_FORMAT_STD) { + tmp32 = cpu_to_be32(simple_strtoul(uuid_str, NULL, 16)); + memcpy(uuid_bin, &tmp32, 4);

- tmp32 = cpu_to_le32(simple_strtoul(uuid, NULL, 16)); - memcpy(out, &tmp32, 4); + tmp16 = cpu_to_be16(simple_strtoul(uuid_str + 9, NULL, 16)); + memcpy(uuid_bin + 4, &tmp16, 2);

- tmp16 = cpu_to_le16(simple_strtoul(uuid + 9, NULL, 16)); - memcpy(out + 4, &tmp16, 2); + tmp16 = cpu_to_be16(simple_strtoul(uuid_str + 14, NULL, 16)); + memcpy(uuid_bin + 6, &tmp16, 2); + } else { + tmp32 = cpu_to_le32(simple_strtoul(uuid_str, NULL, 16)); + memcpy(uuid_bin, &tmp32, 4);

- tmp16 = cpu_to_le16(simple_strtoul(uuid + 14, NULL, 16)); - memcpy(out + 6, &tmp16, 2); + tmp16 = cpu_to_le16(simple_strtoul(uuid_str + 9, NULL, 16)); + memcpy(uuid_bin + 4, &tmp16, 2);

- tmp16 = cpu_to_be16(simple_strtoul(uuid + 19, NULL, 16)); - memcpy(out + 8, &tmp16, 2); + tmp16 = cpu_to_le16(simple_strtoul(uuid_str + 14, NULL, 16)); + memcpy(uuid_bin + 6, &tmp16, 2); + } + + tmp16 = cpu_to_be16(simple_strtoul(uuid_str + 19, NULL, 16)); + memcpy(uuid_bin + 8, &tmp16, 2);

- tmp64 = cpu_to_be64(simple_strtoull(uuid + 24, NULL, 16)); - memcpy(out + 10, (char *)&tmp64 + 2, 6); + tmp64 = cpu_to_be64(simple_strtoull(uuid_str + 24, NULL, 16)); + memcpy(uuid_bin + 10, (char *)&tmp64 + 2, 6);

return 0; }

-void uuid_bin_to_str(unsigned char *uuid, char *str) +/* + * uuid_bin_to_str() - convert big endian binary data to string UUID or GUID. + * + * @param uuid_bin - pointer to binary data of UUID (big endian) [16B] + * @param uuid_str - pointer to allocated array for output string [37B] + * @str_format - UUID string format: 0 - UUID; 1 - GUID + */ +void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, int 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}; + 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 + */ + if (str_format == UUID_STR_FORMAT_STD) + char_order = uuid_char_order; + else + char_order = guid_char_order; + for (i = 0; i < 16; i++) { - sprintf(str, "%02x", uuid[le[i]]); - str += 2; + sprintf(uuid_str, "%02x", uuid_bin[char_order[i]]); + uuid_str += 2; switch (i) { case 3: case 5: case 7: case 9: - *str++ = '-'; + *uuid_str++ = '-'; break; } } diff --git a/net/bootp.c b/net/bootp.c index 4300f1c..189a003 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -439,7 +439,7 @@ static int DhcpExtended(u8 *e, int message_type, IPaddr_t ServerID, *e++ = 17; *e++ = 0; /* type 0 - UUID */

- uuid_str_to_bin(uuid, e); + uuid_str_to_bin(uuid, e, UUID_STR_FORMAT_STD); e += 16; } else { printf("Invalid pxeuuid: %s\n", uuid);

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: - new configs: - CONFIG_LIB_UUID for compile lib/uuid.c - CONFIG_RANDOM_UUID for functions gen_rand_uuid() and gen_rand_uuid_str() - add configs dependency to include/config_fallbacks.h for lib uuid.

lib/uuid.c: - add gen_rand_uuid() - this function writes 16 bytes len binary representation of UUID v4 to the memory at given address.

- add gen_rand_uuid_str() - this function writes 37 bytes len hexadecimal ASCII string representation of UUID v4 to the memory at given address.

Signed-off-by: Przemyslaw Marczak p.marczak@samsung.com Cc: Stephen Warren swarren@nvidia.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: trini@ti.com

--- Changes v2: - put uuid generation changes in a separate commit - get_uuid_str() - change name to gen_rand_uuid_str() - add new function: gen_rand_uuid() - remove unnecessary '\0' at the end of uuid string - drop unnecessary error checking - functions now takes pointers to allocated memory instead of alloc it itself - add new config option: CONFIG_RANDOM_UUID

Changes v3: - remove unused UUID_STR_BYTE_LEN - reword comments - remove null pointer checking from gen_rand_uuid() and gen_rand_uuid_str() - remove unneeded memset from gen_rand_uuid() - undo moving vsprintf.o object in lib/Makefile - add attribute "packed" to the uuid structure - gen_rand_uuid(): add endian functions for modify uuid data - gen_rand_uuid(): use memcpy() to store uuid data into given buffer for avoi unaligned access issues - change uuid version and variant masks to proper for use with clrsetbits_* - add #ifdef CONFIG_RANDOM_UUID to random uuid code for avoid warnings

Changes v4: - add new parameter to define UUID string format for UUID or GUID which differs in endianness of first three string blocks. - add uuid structure and version 4 data to uuid header file - lib/Makefile: add CONFIG_RAND_UUID dependency to rand.o and uuid.o

Changes v5: - reword some comments - introduce config CMD_LIB_UUID, cleanup config dependency - remove use of typedef uuid_str_t

Changes v6: - fix indentation in include/config_fallbacks.h --- include/config_fallbacks.h | 13 +++++++++ include/uuid.h | 22 +++++++++++++++- lib/Makefile | 3 +-- lib/uuid.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h index d8339b2..a8630ab 100644 --- a/include/config_fallbacks.h +++ b/include/config_fallbacks.h @@ -55,6 +55,19 @@ #define HAVE_BLOCK_DEVICE #endif

+#if (defined(CONFIG_PARTITION_UUIDS) || \ + defined(CONFIG_RANDOM_UUID) || \ + defined(CONFIG_BOOTP_PXE)) && \ + !defined(CONFIG_LIB_UUID) +#define CONFIG_LIB_UUID +#endif + +#if defined(CONFIG_RANDOM_UUID) && \ + !defined(CONFIG_LIB_RAND) && \ + !defined(CONFIG_LIB_HW_RAND) +#define CONFIG_LIB_RAND +#endif + #ifndef CONFIG_SYS_PROMPT #define CONFIG_SYS_PROMPT "=> " #endif diff --git a/include/uuid.h b/include/uuid.h index e8feeed..93027c1 100644 --- a/include/uuid.h +++ b/include/uuid.h @@ -7,15 +7,35 @@ #ifndef __UUID_H__ #define __UUID_H__

+/* This is structure is in big-endian */ +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]; +} __packed; + enum { UUID_STR_FORMAT_STD, UUID_STR_FORMAT_GUID };

#define UUID_STR_LEN 36 -#define UUID_BIN_LEN 16 +#define UUID_BIN_LEN sizeof(struct uuid) + +#define UUID_VERSION_MASK 0xf000 +#define UUID_VERSION_SHIFT 12 +#define UUID_VERSION 0x4 + +#define UUID_VARIANT_MASK 0xc0 +#define UUID_VARIANT_SHIFT 7 +#define UUID_VARIANT 0x1

int uuid_str_valid(const char *uuid); int uuid_str_to_bin(char *uuid_str, unsigned char *uuid_bin, int str_format); void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, int str_format); +void gen_rand_uuid(unsigned char *uuid_bin); +void gen_rand_uuid_str(char *uuid_str, int str_format); #endif diff --git a/lib/Makefile b/lib/Makefile index d7ff7ca..27e4f78 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -60,8 +60,7 @@ obj-$(CONFIG_REGEX) += slre.o obj-y += string.o obj-y += time.o obj-$(CONFIG_TRACE) += trace.o -obj-$(CONFIG_BOOTP_PXE) += uuid.o -obj-$(CONFIG_PARTITION_UUIDS) += uuid.o +obj-$(CONFIG_LIB_UUID) += uuid.o obj-y += vsprintf.o obj-$(CONFIG_LIB_RAND) += rand.o

diff --git a/lib/uuid.c b/lib/uuid.c index 8db865f..44d0c93 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -14,8 +14,22 @@ /* * 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. A UUID contains a set of fields. The set varies + * depending on the version of the UUID, as shown below: + * - time, MAC address(v1), + * - user ID(v2), + * - MD5 of name or URL(v3), + * - random data(v4), + * - SHA-1 of name or URL(v5), + * + * Layout of UUID: + * 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 + * + * source: https://www.ietf.org/rfc/rfc4122.txt * * UUID binary format (16 bytes): * @@ -149,3 +163,51 @@ void uuid_bin_to_str(unsigned char *uuid_bin, char *uuid_str, int str_format) } } } + +/* + * gen_rand_uuid() - this function generates a random binary UUID version 4. + * In this version all fields beside 4 bits of version and + * 2 bits of variant are randomly generated. + * + * @param uuid_bin - pointer to allocated array [16B]. Output is in big endian. +*/ +#ifdef CONFIG_RANDOM_UUID +void gen_rand_uuid(unsigned char *uuid_bin) +{ + struct uuid uuid; + unsigned int *ptr = (unsigned int *)&uuid; + int i; + + /* Set all fields randomly */ + for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++) + *(ptr + i) = cpu_to_be32(rand()); + + clrsetbits_be16(&uuid.time_hi_and_version, + UUID_VERSION_MASK, + UUID_VERSION << UUID_VERSION_SHIFT); + + clrsetbits_8(&uuid.clock_seq_hi_and_reserved, + UUID_VARIANT_MASK, + UUID_VARIANT << UUID_VARIANT_SHIFT); + + memcpy(uuid_bin, &uuid, sizeof(struct uuid)); +} + +/* + * gen_rand_uuid_str() - this function generates UUID v4 (random) in two string + * formats UUID or GUID. + * + * @param uuid_str - pointer to allocated array [37B]. + * @param - uuid output type: UUID - 0, GUID - 1 + */ +void gen_rand_uuid_str(char *uuid_str, int str_format) +{ + unsigned char uuid_bin[UUID_BIN_LEN]; + + /* Generate UUID (big endian) */ + gen_rand_uuid(uuid_bin); + + /* Convert UUID bin to UUID or GUID formated STRING */ + uuid_bin_to_str(uuid_bin, uuid_str, str_format); +} +#endif