lib: uuid: add function to generate UUID version 4

Quoted 5 lines · Przemyslaw Marczak

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

common/cmd_gpt.c line 34

Quoted 5 lines

common/cmd_gpt.c +1 -0

 
  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.

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

Quoted 5 lines

common/cmd_gpt.c +0 -1

@@ -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);

common/cmd_gpt.c static char extract_env(const char *str, char **env) · line 52

Quoted 8 lines

common/cmd_gpt.c +5 -2

  		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);

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

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

Quoted 4 lines

common/cmd_gpt.c +0 -0

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

common/cmd_gpt.c static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) · line 315

Quoted 7 lines

common/cmd_gpt.c +6 -1

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

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.

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.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote:

[snip]

Quoted 11 lines

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.

Having been using this code again myself recently, at the high level, this is useful. Having to copy/paste in N UUIDs gets silly. But I also wonder..

[snip]

Quoted 4 lines

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.

Is this really an important use case to cover?

The way I see things, would it be possible (and not a pain) to make the UUID part of the partition string passed to 'gpt write' optional. If not passed, generate the UUIDs needed. What was used would be seen in 'part list' and so forth.

- --
Tom

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTFI32AAoJENk4IS6UOR1WAyQP/2bfClvRaWtLzUDggRUnNr99 RKSgB1m/nl09ZwclB2Gn1FZL2IR/M0u+ia89FE9leSN4eoOEswN2rrUiExVv7QpJ d+oCH/H3oNDAyb9L4HXXYHeRGCVHK7KE/KP5Ngb7KfTGZhj5kHdkx9iQM7dO9OtX DmvW5+JoRXgXPkpmvy5s20IzRUbBEqGC6Z2h8Y/VKHTDnrUmYvFb6XPgjwwrYVB6 +Hx/7K2R1uXQTuoHM7FqVAqKx7GGiUmcpLG90iHeYcX4fib/+RwC7hcre0rRj71v NyAnO4t0nXKefLXYe5iGold7cNx9xUKO3s4u2EkC9lNGRkN6RcVuaxzEKGD4/QTX fHA2q6FwSVLv4lXBT6UWxXiky2C9TMY/DUNM0EWe2KSx2V2glXPKiBX7gRk0Rq0d EWq1G5oQS0qiZgb+vUQ4Acf4/HDjhAl1l18Hx6w+26LNvz8Fi+o7Om4kkBdI5sos GX2auS1RxQc3qQkrAjxFx1xpDr9iMikV6nNqYDcVU894PeCgzmUWaWG/IrBFixO6 2XjF1sMth8LrVqUHirh3Y7lDU+FFPP+mMb6eaY3oajtVkg3u6cpQ0eJ0A612CsXG oaTPxWKKTsWlBxaVIgzu2OXeYg5BJF4OTKxjNiV2wptheEVc4RVLDiQ1yf74YEHy Hve7lpxa9i4Oo9Vf2Pd7
=5FKf
-----END PGP SIGNATURE-----

Hello Tom,

Quoted 28 lines · Tom Rini

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote:

[snip]

Quoted 11 lines

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.

Having been using this code again myself recently, at the high level, this is useful. Having to copy/paste in N UUIDs gets silly. But I also wonder..

[snip]

Quoted 4 lines

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.

Quoted 1 line

Is this really an important use case to cover?

It can be important if somebody uses UUIDS to boot kernel. In kernel documentation you can find a notice about kernel function name_to_dev_t() - so by command line you can pass uuid for root partition. And the same is for arg "suspend" in kernel cmd line.

Quoted 4 lines

The way I see things, would it be possible (and not a pain) to make the UUID part of the partition string passed to 'gpt write' optional. If not passed, generate the UUIDs needed. What was used would be seen in 'part list' and so forth.

Ok, so I remove saveenv() from my changes and then we will have two cases:

# gpt write mmc 0 $partitions
case 1: envorinment uuids are not set; then:
  proper uuids variables are set automatically (and printed)

case 2: environment uuids are set in env (e.g. some user has put his own env); then
users uuids will be used and new uuids are not generated automatically

So this will not change current "gpt" usability - just add new feature, moreover user will be informed about each uuid generation. In case when someone use gpt write by mistake and overwrite uuids by randomly generated then he can easily back to his own uuids by setting each in environment and run gpt write again.

Quoted 31 lines · Przemyslaw Marczak

Hello Tom,

Quoted 28 lines · Tom Rini

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote:

[snip]

Quoted 11 lines

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.

Having been using this code again myself recently, at the high level, this is useful. Having to copy/paste in N UUIDs gets silly. But I also wonder..

[snip]

Quoted 4 lines

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.

Quoted 6 lines

Quoted 1 line

Is this really an important use case to cover?

It can be important if somebody uses UUIDS to boot kernel. In kernel documentation you can find a notice about kernel function name_to_dev_t() - so by command line you can pass uuid for root partition. And the same is for arg "suspend" in kernel cmd line.

Right. But that seems to be putting things in the wrong order. If you need to restore UUIDs to your partition table, you pass in the optional and already known UUID. If you're starting from scratch, by the time the installer is run U-Boot is long gone. And tying things back to the commodity distro stuff, we would be fetching 'root=UUID=...' from some file generated and controlled on the Linux side of things anyhow. To be clear, on the OS side of the equation there's much better ways to find out that partition1 has a UUID of ... than poking the U-Boot environment.

Quoted 7 lines

Quoted 4 lines

The way I see things, would it be possible (and not a pain) to make the UUID part of the partition string passed to 'gpt write' optional. If not passed, generate the UUIDs needed. What was used would be seen in 'part list' and so forth.

Ok, so I remove saveenv() from my changes and then we will have two cases:

Quoted 3 lines

# gpt write mmc 0 $partitions
case 1: envorinment uuids are not set; then:
   proper uuids variables are set automatically (and printed)

I'd go so far as to say we don't need to print the uuids, they're available via 'part list ...'. A notice that we're generating UUIDs is probably not too spammy.

Quoted 9 lines

case 2: environment uuids are set in env (e.g. some user has put his own env); then
users uuids will be used and new uuids are not generated automatically

So this will not change current "gpt" usability - just add new feature, moreover user will be informed about each uuid generation. In case when someone use gpt write by mistake and overwrite uuids by randomly generated then he can easily back to his own uuids by setting each in environment and run gpt write again.

Right. We're making the use case of "fresh device, create new table" easier and we're still allowing "existing device, re-establish old UUIDs".

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 41 lines · Tom Rini

Quoted 33 lines · Przemyslaw Marczak

Hello Tom,

Quoted 30 lines · Tom Rini

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote:

[snip]

Quoted 4 lines

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.

Quoted 8 lines

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.

Having been using this code again myself recently, at the high level, this is useful. Having to copy/paste in N UUIDs gets silly. But I also
wonder..

[snip]

Quoted 4 lines

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.

Quoted 6 lines

Quoted 1 line

Is this really an important use case to cover?

It can be important if somebody uses UUIDS to boot kernel. In kernel documentation you can find a notice about kernel function name_to_dev_t() - so by command line you can pass uuid for root partition. And the same is for arg "suspend" in kernel cmd line.

Quoted 10 lines

Right. But that seems to be putting things in the wrong order. If you need to restore UUIDs to your partition table, you pass in the optional and already known UUID. If you're starting from scratch, by the time the installer is run U-Boot is long gone. And tying things back to the commodity distro stuff, we would be fetching 'root=UUID=...' from some file generated and controlled on the Linux side of things anyhow. To be clear, on the OS side of the equation there's much better ways to find out that partition1 has a UUID of ... than poking the U-Boot environment.

Ok, I understand this.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

The way I see things, would it be possible (and not a pain) to make the UUID part of the partition string passed to 'gpt write' optional. If not passed, generate the UUIDs needed. What was used would be seen in 'part list' and so forth.

Ok, so I remove saveenv() from my changes and then we will have two cases:

Quoted 8 lines

Quoted 3 lines

# gpt write mmc 0 $partitions
case 1: envorinment uuids are not set; then:
   proper uuids variables are set automatically (and printed)

I'd go so far as to say we don't need to print the uuids, they're available via 'part list ...'. A notice that we're generating UUIDs is probably not too spammy.

Ok, I will remove this notice.

Quoted 11 lines

Quoted 10 lines

case 2: environment uuids are set in env (e.g. some user has put his own env); then
users uuids will be used and new uuids are not generated
automatically

So this will not change current "gpt" usability - just add new feature, moreover user will be informed about each uuid generation. In case when someone use gpt write by mistake and overwrite uuids by randomly generated then he can easily back to his own uuids by setting each in environment and run gpt write again.

Quoted 4 lines

Right. We're making the use case of "fresh device, create new table" easier and we're still allowing "existing device, re-establish old UUIDs".

That's the point.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 85 lines · Przemyslaw Marczak

Quoted 41 lines · Tom Rini

Quoted 33 lines · Przemyslaw Marczak

Hello Tom,

Quoted 30 lines · Tom Rini

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote:

[snip]

Quoted 4 lines

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.

Quoted 8 lines

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.

Having been using this code again myself recently, at the high level, this is useful. Having to copy/paste in N UUIDs gets silly. But I also
wonder..

[snip]

Quoted 4 lines

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.

Quoted 6 lines

Quoted 1 line

Is this really an important use case to cover?

It can be important if somebody uses UUIDS to boot kernel. In kernel documentation you can find a notice about kernel function name_to_dev_t() - so by command line you can pass uuid for root partition. And the same is for arg "suspend" in kernel cmd line.

Quoted 10 lines

Right. But that seems to be putting things in the wrong order. If you need to restore UUIDs to your partition table, you pass in the optional and already known UUID. If you're starting from scratch, by the time the installer is run U-Boot is long gone. And tying things back to the commodity distro stuff, we would be fetching 'root=UUID=...' from some file generated and controlled on the Linux side of things anyhow. To be clear, on the OS side of the equation there's much better ways to find out that partition1 has a UUID of ... than poking the U-Boot environment.

Ok, I understand this.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

The way I see things, would it be possible (and not a pain) to make the UUID part of the partition string passed to 'gpt write' optional. If not passed, generate the UUIDs needed. What was used would be seen in 'part list' and so forth.

Ok, so I remove saveenv() from my changes and then we will have two cases:

Quoted 8 lines

Quoted 3 lines

# gpt write mmc 0 $partitions
case 1: envorinment uuids are not set; then:
    proper uuids variables are set automatically (and printed)

I'd go so far as to say we don't need to print the uuids, they're available via 'part list ...'. A notice that we're generating UUIDs is probably not too spammy.

Ok, I will remove this notice.

Quoted 11 lines

Quoted 10 lines

case 2: environment uuids are set in env (e.g. some user has put his own env); then
users uuids will be used and new uuids are not generated
automatically

So this will not change current "gpt" usability - just add new feature, moreover user will be informed about each uuid generation. In case when someone use gpt write by mistake and overwrite uuids by randomly generated then he can easily back to his own uuids by setting each in environment and run gpt write again.

Quoted 5 lines

Quoted 4 lines

Right. We're making the use case of "fresh device, create new table" easier and we're still allowing "existing device, re-establish old UUIDs".

That's the point.

Yay for agreement then, even if it took a few rounds to see we're on the same side :) I look forward to v2.

- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTFL1DAAoJENk4IS6UOR1WT0EP/12dTZmJGcgZYPgIaRohWAjS Dk3n0PdkPszGic/n5j6un6b8gSck27cTI3aM5YvqRioRBTzw6tvGVoIBOfrtEKdZ EV78iqMq1cozL3o3VdltQlLAzdLzSMwkkEqdD9K4vWpihdr19PPPuaPlV0ynw4/D 6+a1azEqTtznx96bGIH0XLK37ZtVre0/n1+9nd+mQ/swibHQM84GNPdR1L5m3hnJ W80XCnX4wRh238KxXRKr/fqG5w9cVXFcsPWESXzfA+pzi9Jy37dkhMX3CyAGuwS0 QDLz1gg4h6CYGCRulN6rfNYHbLg8YYbJ/5xa4/dSntUW84uZJRMq93T9yKYX+qKM Iezp+EKzoL3Lagz3K2GmwopazEd+0goEHauoQ8QUGDdgMGapXui5NBUNyke78JK8 U/WszX6JOWjagenWHDYiBvPhwIWdGw3FUJVijutKhK8KpKKihC9eDOsdoxz+Lyfu agcZ8rtJ8s6QBEf+xo/PRS+twJeHuTe/CptORm52cXxPMrwOKK17UxrjLJIuDSDl fqVsUaUHmxFmUNU/k5SGI60d/nZGmi7j8EzMMRIK8DJ18Sk0FaBOj/PKo2xBf3RO afbnEQXi831j42V2HSOEEY6rKRzES9d0roSUFIe5EvP/s/1/O1OanimtrXRISjF9 rKXW1zAJZKQlBbYoDOSa
=eGvG
-----END PGP SIGNATURE-----

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

+int uuid_bin_to_str(unsigned char *uuid, char *str);

lib/uuid.c +8 -0

 
+#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

lib/uuid.c +7 -0

+/*
+ * 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:

Quoted 8 lines · Przemyslaw Marczak

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

disk/part_efi.c +0 -0

disk/part_efi.c void print_part_efi(block_dev_desc_t * dev_desc) · line 117

Quoted 7 lines

disk/part_efi.c +2 -1

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

disk/part_efi.c int get_partition_info_efi(block_dev_desc_t * dev_desc, int part, · line 168

Quoted 6 lines

disk/part_efi.c +1 -1

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

include/common.h void udelay (unsigned long); · line 818

Quoted 5 lines

include/common.h +1 -1

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

Quoted 1 line

+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?

include/common.h char * strmhz(char *buf, unsigned long hz); · line 836

Quoted 6 lines

  /* lib/rand.c */
  #if defined(CONFIG_RANDOM_MACADDR) || \
  	defined(CONFIG_BOOTP_RANDOM_DELAY) || \

include/common.h +2 -1

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

Also, not everyone who defines CONFIG_PARTITION_UUIDs needs the new get_uuid_str() function, and hence not everyone needs rand() etc.

lib/Makefile obj-y += vsprintf.o · line 71

Quoted 6 lines

lib/Makefile +4 -0

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

Quoted 15 lines

lib/uuid.c +13 -0

 
+#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;

lib/uuid.c line 30

Quoted 5 lines

lib/uuid.c +5 -0

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

Quoted 7 lines

lib/uuid.c +1 -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;

lib/uuid.c int uuid_str_valid(const char *uuid) · line 78

Quoted 6 lines

  	if (!uuid || !out)

lib/uuid.c +4 -1

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

Quoted 13 lines

+/*
+ * 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.

Quoted 4 lines

+ *
+ * @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.

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.

lib/uuid.c void uuid_str_to_bin(const char *uuid, unsigned char *out) · line 149

Quoted 10 lines

lib/uuid.c +9 -0

+{
+	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?

lib/uuid.c void uuid_str_to_bin(const char *uuid, unsigned char *out) · line 157

Quoted 3 lines

lib/uuid.c +3 -0

+	/* 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).

lib/uuid.c void uuid_str_to_bin(const char *uuid, unsigned char *out) · line 166

Quoted 1 line

lib/uuid.c +1 -0

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

lib/uuid.c void uuid_str_to_bin(const char *uuid, unsigned char *out) · line 171

Quoted 5 lines

lib/uuid.c +4 -0

+	if (!uuid_str_valid(uuid_str)) {
+		error("Invalid UUID string");
+		return NULL;
+	}

Isn't that code already part of uuid_bin_to_str()?

lib/uuid.c void uuid_str_to_bin(const char *uuid, unsigned char *out) · line 174

Quoted 3 lines

lib/uuid.c +2 -0

+	/* 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.

Hello Stephen,
Thank you for review.

Quoted 11 lines · Stephen Warren

Quoted 9 lines · Przemyslaw Marczak

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

disk/part_efi.c +0 -0

Quoted 11 lines

disk/part_efi.c void print_part_efi(block_dev_desc_t * dev_desc) · line 117

Quoted 7 lines

disk/part_efi.c +2 -1

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

Quoted 5 lines

disk/part_efi.c void print_part_efi(block_dev_desc_t * dev_desc) · line 118

Quoted 1 line

disk/part_efi.c +1 -0

+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)

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

Signed-off-by Przemyslaw Marczak <[email protected]>

cc Stephen Warren <[email protected]>

cc trini at ti.com

lib/Makefile +1 -0

+obj-$(CONFIG_RANDOM_UUID) += rand.o
 obj-$(CONFIG_RANDOM_MACADDR) += rand.o

lib/uuid.c +0 -0

index 803bdcd..c0218ba 100644

+/*
+ * 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.

Quoted 5 lines · Przemyslaw Marczak

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

Quoted 7 lines

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.

Quoted 3 lines

Signed-off-by Przemyslaw Marczak <[email protected]>

cc Stephen Warren <[email protected]>

cc trini at ti.com

s/cc/Cc/

lib/Makefile obj-y += linux_string.o · line 62

Quoted 9 lines

lib/Makefile +1 -0

index 70962b2..64a430f 100644
@@ -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.

lib/Makefile obj-y += linux_string.o · line 68

Quoted 7 lines

lib/Makefile +2 -1

  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

lib/Makefile obj-y += linux_string.o · line 70

Quoted 2 lines

lib/Makefile +1 -0

  obj-$(CONFIG_RANDOM_MACADDR) += rand.o
+obj-$(CONFIG_RANDOM_UUID) += rand.o

rather than:

Quoted 8 lines

lib/Makefile +1 -0

+obj-$(CONFIG_RANDOM_UUID) += rand.o
  obj-$(CONFIG_RANDOM_MACADDR) += rand.o

lib/uuid.c +0 -0

index 803bdcd..c0218ba 100644

lib/uuid.c line 14

Quoted 8 lines

lib/uuid.c +5 -0

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

lib/uuid.c line 15

Quoted 1 line

lib/uuid.c +1 -0

+#define UUID_BIN_BYTE_LEN		16

Also, s/_BYTE// there.

lib/uuid.c line 17

Quoted 1 line

lib/uuid.c +1 -0

+#define UUID_VERSION_CLEAR_BITS		0x0fff

s/CLEAR_BITS/MASK/

lib/uuid.c line 32

Quoted 8 lines

lib/uuid.c +8 -0

+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?

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 123

Quoted 3 lines

lib/uuid.c +3 -0

+/*
+ * 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.

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 143

Quoted 8 lines

lib/uuid.c +8 -0

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

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 149

Quoted 5 lines

lib/uuid.c +5 -0

+	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?

Quoted 5 lines

+/*
+ * 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.

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 172

Quoted 6 lines

lib/uuid.c +6 -0

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

Quoted 2 lines

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

This patch adds support to generate UUID (Universally Unique Identifier) in version 4 based on RFC4122, which is randomly.

Dear Przemyslaw Marczak,

Quoted 2 lines · In message <cc0f558724a4d3ea3497b84601038f5f18f37a7b.1394037321.git.p.marczak@s…

This patch adds support to generate UUID (Universally Unique Identifier) in version 4 based on RFC4122, which is randomly.

...

lib/uuid.c line 32

Quoted 8 lines

lib/uuid.c +8 -0

+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];
+};

This struct starts with an uint, so it requires alignment on a 32 bit boundary (i. e. an address that is a multiple of 4).

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 138

Quoted 3 lines

lib/uuid.c +3 -0

+void gen_rand_uuid(unsigned char *uuid_bin)
+{
+	struct uuid *uuid = (struct uuid *)uuid_bin;

Here you cast a pointer to the (unaligned) character buffer to a struct buffer, which requires alignment.

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 149

Quoted 5 lines

lib/uuid.c +4 -0

+	unsigned int *ptr = (unsigned int *)uuid_bin;
 
+	/* Set all fields randomly */
+	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
+		*(ptr + i) = rand();

This code is dangerous - if the size of the struct should not be a multiple of sizeof(uint), there would remain uninitialized data.

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 153

Quoted 3 lines

lib/uuid.c +3 -0

+	/* Set V4 format */
+	uuid->time_hi_and_version &= UUID_VERSION_CLEAR_BITS;
+	uuid->time_hi_and_version |= UUID_VERSION << UUID_VERSION_SHIFT;

Potentially unaligned accesses.

Quoted 12 lines

lib/uuid.c line 32

Quoted 9 lines

lib/uuid.c +8 -0

+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];
+};

This struct starts with an uint, so it requires alignment on a 32 bit boundary (i. e. an address that is a multiple of 4).

And this needs to be marked as packed since we're using this as a direct representation of things on-disk.

Quoted 6 lines

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 138

Quoted 3 lines

lib/uuid.c +3 -0

+void gen_rand_uuid(unsigned char *uuid_bin)
+{
+	struct uuid *uuid = (struct uuid *)uuid_bin;

Here you cast a pointer to the (unaligned) character buffer to a struct buffer, which requires alignment.

Potentially unaligned buffer. There's only one caller thus far, and it will be aligned there. We do need to comment that the pointer needs to be aligned.

With the struct not packed, it'll be padded out so this works. But looking at how we later use this as I say above, we do need to pack it, and then this will not be safe. Some looping of strncpy into the char buffer, as a char so we whack the rand data in?

Dear Tom,

Quoted 17 lines · In message <20140313191814.GB16360@bill-the-cat> you wrote:

Quoted 16 lines

lib/uuid.c

Quoted 13 lines

lib/uuid.c +11 -0

+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];
+};
 
+void gen_rand_uuid(unsigned char *uuid_bin)
+{
+	struct uuid *uuid = (struct uuid *)uuid_bin;

Here you cast a pointer to the (unaligned) character buffer to a struct buffer, which requires alignment.

Quoted 3 lines

Potentially unaligned buffer. There's only one caller thus far, and it will be aligned there. We do need to comment that the pointer needs to be aligned.

No. We should rather fix the code such that it works with any alignment. It is trivial here to use a struct uuid on the stack and then use memcpy() to cpy the data from the struct to the buffer - no casting needed anywhere, and no alignment concerns either.

Quoted 6 lines · Wolfgang Denk

Dear Przemyslaw Marczak,

Quoted 2 lines · In message <cc0f558724a4d3ea3497b84601038f5f18f37a7b.1394037321.git.p.marczak@s…

This patch adds support to generate UUID (Universally Unique Identifier) in version 4 based on RFC4122, which is randomly.

...

Quoted 11 lines

lib/uuid.c line 32

Quoted 8 lines

lib/uuid.c +8 -0

+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];
+};

This struct starts with an uint, so it requires alignment on a 32 bit boundary (i. e. an address that is a multiple of 4).

And this needs to be marked as packed since we're using this as a direct representation of things on-disk.

Quoted 1 line

Quoted 1 line

+void gen_rand_uuid(unsigned char *uuid_bin)

Quoted 5 lines

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 138

Quoted 2 lines

lib/uuid.c +2 -0

+{
+	struct uuid *uuid = (struct uuid *)uuid_bin;

Here you cast a pointer to the (unaligned) character buffer to a struct buffer, which requires alignment.

Potentially unaligned buffer. There's only one caller thus far, and it will be aligned there. We do need to comment that the pointer needs to be aligned.

Quoted 8 lines

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 149

Quoted 5 lines

lib/uuid.c +4 -0

+	unsigned int *ptr = (unsigned int *)uuid_bin;
 
+	/* Set all fields randomly */
+	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
+		*(ptr + i) = rand();

This code is dangerous - if the size of the struct should not be a multiple of sizeof(uint), there would remain uninitialized data.

With the struct not packed, it'll be padded out so this works. But looking at how we later use this as I say above, we do need to pack it, and then this will not be safe. Some looping of strncpy into the char buffer, as a char so we whack the rand data in?

Quoted 5 lines

lib/uuid.c void uuid_bin_to_str(unsigned char *uuid, char *str) · line 153

Quoted 3 lines

lib/uuid.c +3 -0

+	/* Set V4 format */
+	uuid->time_hi_and_version &= UUID_VERSION_CLEAR_BITS;
+	uuid->time_hi_and_version |= UUID_VERSION << UUID_VERSION_SHIFT;

Potentially unaligned accesses.

As-is no (hidden padding), with packed yes-but-handled (compiler can see this too, will behave correctly).

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

common/cmd_gpt.c +3 -2

 
-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);

Quoted 15 lines · Przemyslaw Marczak

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

common/cmd_gpt.c +3 -1

 
-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];

Quoted 4 lines

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

common/cmd_gpt.c +0 -0

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

common/cmd_gpt.c static char extract_env(const char *str, char **env) · line 52

Quoted 10 lines

common/cmd_gpt.c +4 -3

  		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);
+

common/cmd_gpt.c static char extract_env(const char *str, char **env) · line 55

Quoted 3 lines

common/cmd_gpt.c +3 -0

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

doc/README.gpt Please, pay attention@-l switch for parted. · line 179

Quoted 7 lines

doc/README.gpt +1 -0

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

Quoted 9 lines · Stephen Warren

Quoted 7 lines · Przemyslaw Marczak

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

common/cmd_gpt.c +0 -0

Quoted 12 lines

common/cmd_gpt.c static char extract_env(const char *str, char **env) · line 52

Quoted 12 lines

common/cmd_gpt.c +4 -3

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

Quoted 4 lines

Quoted 3 lines

common/cmd_gpt.c +3 -0

+            e = getenv(s);
+            if (e) {
+                puts("Setting to random.\n");

Quoted 5 lines

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.

Review

Quoted 5 lines

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

(quoted diff) +2 -0

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

Quoted 29 lines · Przemyslaw Marczak

Quoted 9 lines · Stephen Warren

Quoted 7 lines · Przemyslaw Marczak

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

common/cmd_gpt.c +0 -0

Quoted 12 lines

common/cmd_gpt.c static char extract_env(const char *str, char **env) · line 52

Quoted 12 lines

common/cmd_gpt.c +4 -3

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

Quoted 4 lines

Quoted 3 lines

common/cmd_gpt.c +3 -0

+            e = getenv(s);
+            if (e) {
+                puts("Setting to random.\n");

Quoted 7 lines

Quoted 5 lines

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;
	}

}

With the code in your patch, the following happens:

* Env var is set: Nothing printed.

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

Quoted 11 lines

Review

Quoted 5 lines

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

(quoted diff) +2 -0

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

Quoted 5 lines

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?

Changes:
Move functions:
- disk/part_efi.c uuid_string() to lib/uuid.c -> uuid_bin_to_str()
- disk/part_efi.c string_uuid() to lib/uuid.c -> uuid_str_to_bin()

Update files:
- include/common.h
- disk/part_efi.c
- lib/Makefile

Quoted 9 lines · Przemyslaw Marczak

Changes:
Move functions:
- disk/part_efi.c uuid_string() to lib/uuid.c -> uuid_bin_to_str()
- disk/part_efi.c string_uuid() to lib/uuid.c -> uuid_str_to_bin()

Update files:
- include/common.h
- disk/part_efi.c
- lib/Makefile

That's not a particularly useful patch description. How about:

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(), so that's the naming is consistent with the existing uuid_str_to_bin(). Add an error return code to uuid_str_to_bin(). Convert existing users to the new library functions.

This one patch,

Acked-by Stephen Warren <[email protected]>

lib/Makefile obj-y += string.o · line 64

Quoted 4 lines

lib/Makefile +1 -0

 
 obj-$(CONFIG_BOOTP_PXE) += uuid.o
+obj-$(CONFIG_PARTITION_UUIDS) += uuid.o

Oh, I hope listing uuid.o twice in obj-y won't cause any issue...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 13 lines · Stephen Warren

Quoted 9 lines · Przemyslaw Marczak

Changes:
Move functions:
- disk/part_efi.c uuid_string() to lib/uuid.c -> uuid_bin_to_str()
- disk/part_efi.c string_uuid() to lib/uuid.c -> uuid_str_to_bin()

Update files:
- include/common.h
- disk/part_efi.c
- lib/Makefile

That's not a particularly useful patch description. How about:

Quoted 8 lines

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(), so that's the naming is consistent with the existing uuid_str_to_bin(). Add an error return code to uuid_str_to_bin(). Convert existing users to the new library functions.

This one patch,

Acked-by Stephen Warren <[email protected]>

As I'm applying and testing this locally right now, I'll take that better wording (and ack), thanks!

Quoted 2 lines

Quoted 1 line

lib/Makefile +0 -0

Quoted 4 lines

Quoted 2 lines

obj-$(CONFIG_BOOTP_PXE) += uuid.o
+obj-$(CONFIG_PARTITION_UUIDS) += uuid.o

Oh, I hope listing uuid.o twice in obj-y won't cause any issue...

Nope, we're good, it gets filtered now. We should handle this better once we have Kconfig in, but, one step at a time.

Changes:
Move functions:
- disk/part_efi.c uuid_string() to lib/uuid.c -> uuid_bin_to_str()
- disk/part_efi.c string_uuid() to lib/uuid.c -> uuid_str_to_bin()

lib/uuid.c +1 -1

 
-void uuid_str_to_bin(const char *uuid, unsigned char *out)
+int uuid_str_to_bin(char *uuid, unsigned char *out)
 {

Quoted 14 lines · Przemyslaw Marczak

Changes:
Move functions:
- disk/part_efi.c uuid_string() to lib/uuid.c -> uuid_bin_to_str()
- disk/part_efi.c string_uuid() to lib/uuid.c -> uuid_str_to_bin()

lib/uuid.c +1 -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;

lib/uuid.c int uuid_str_valid(const char *uuid) · line 57

Quoted 6 lines

lib/uuid.c +4 -1

 	if (!uuid || !out)
-		return;
+		return -EINVAL;
+
+	if (strlen(uuid) != UUID_STR_LEN)
+		return -EINVAL;

Oh. This patch won't compile, since UUID_STR_LEN doesn't exist yet.

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

+#define UUID_BIN_LEN 16

Dear Przemyslaw Marczak,

Quoted 4 lines · In message <bf85640549b0798b838145e9bad6dcc59454e7ae.1394807506.git.p.marczak@s…

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

...

Quoted 1 line

+#define UUID_BIN_LEN 16

...

lib/uuid.c line 33

Quoted 8 lines

lib/uuid.c +8 -0

+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;

Maybe we should define UUID_BIN_LEN as sizeof(struct uuid) ?

Best regards,

Wolfgang Denk

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