diff mbox

[U-Boot,V2,1/3] part_efi: move uuid_string() and string_uuid() to lib/uuid.c

Message ID afb7d9d334d463a843c4615e67661e9fcae5b709.1394037321.git.p.marczak@samsung.com
State Changes Requested
Delegated to: Tom Rini
Headers show

Commit Message

Przemyslaw Marczak March 5, 2014, 4:45 p.m. UTC 
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

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Acked-by: Lukasz Majewski <l.majewski@samsung.com>
cc: Stephen Warren <swarren@nvidia.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
---
 disk/part_efi.c  |   90 +++++++-----------------------------------------------
 include/common.h |    3 +-
 lib/Makefile     |    1 +
 lib/uuid.c       |   33 ++++++++++++++++++--
 4 files changed, 44 insertions(+), 83 deletions(-)

Comments

Stephen Warren March 10, 2014, 5:24 p.m. UTC | #1 
On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:
> 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 <swarren@nvidia.com>

> diff --git a/lib/Makefile b/lib/Makefile

>  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...
Tom Rini March 10, 2014, 5:28 p.m. UTC | #2 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/10/2014 01:24 PM, Stephen Warren wrote:
> On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:
>> 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 <swarren@nvidia.com>

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

>> diff --git a/lib/Makefile b/lib/Makefile
> 
>>  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.

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

iQIcBAEBAgAGBQJTHfY/AAoJENk4IS6UOR1W71YP/2tunn6ln8Qu3AG4YvcbSnQz
FXESBryhXX4dzBnopwLRTi09zjfLQsT8a/uRpfPbo3OF7zUanoLCF11/eQK5k+Q9
3OcorHX98rozhC6/eyJfF7rzuznOKdh7IL+q+v715jll8riQAo103WVP8kPnjeoN
KvZjl80ZLqX7qVTj4678wPQPJpjDFXF7uHrChfsEd3+XK31+MZrt58L58+VPARHA
43UghkcXGIP5ybFpi/h1N0b8Pu1AjOHC+WgHeUjdjfFiLL4laVFc3eVrPjsSltXf
7Ooq6tlFL9YtmtFToW/Ek2OeNN547bFxC7eVEMgzamUjFpYCTFh9E8ISTmhcJBK4
dnMNQ6obtVSZDYZD1PkPEpLvj48aKByXm2YIgdOo8Ds/Vza4g59VuCw6gFLPqQUz
sR/+xr79GOTK+17ayTkacL0SS3FKbSWTSEtseLltaGtDOGE7UoWh8ENzmm8GwVdH
rmxuR6ebYlLpINgT6oUvIA2i87vAZgjbaZMGzcC3SzI3ehakid3UfX7XBJ4xLJWj
BNtuBq/k43SGsuC3IYAIl1HbQZiAKrQZhlqy80xp2EXp5MbSvEfaCnTV0sCHRWH4
fYoJ8v1ztHVn6Z8QfUL3mvJwR3BRd3UY/R4TD62/A054qPjisu8foFjpcrA+NrRq
+OI1PLXI/hF5DG5gVsgM
=pbb1
-----END PGP SIGNATURE-----
Stephen Warren March 10, 2014, 5:29 p.m. UTC | #3 
On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:
> 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()

> diff --git a/lib/uuid.c b/lib/uuid.c

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

Oh. This patch won't compile, since UUID_STR_LEN doesn't exist yet.
Tom Rini March 10, 2014, 5:39 p.m. UTC | #4 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/10/2014 01:29 PM, Stephen Warren wrote:
> On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:
>> 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()
> 
>> diff --git a/lib/uuid.c b/lib/uuid.c
> 
>> -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;
> 
> Oh. This patch won't compile, since UUID_STR_LEN doesn't exist yet.

Yup, made this '36' and fixed in 2/3.

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

iQIcBAEBAgAGBQJTHfi/AAoJENk4IS6UOR1Wf9QQAIpOqG7zaURNl4I8k+9wRTe+
M9qJB9a98diBFzUzFBwciwl3xZvXEDnxTyZmoXOCWLjOn46PCqCrufSj9A7F9vvC
XtOm5SQPJsteq8d2wsIqyg6CzkAv4fnQRAVQIsyY3cvOojLkqqU6tSZxI1ojNOea
FBEI0XzHmAb1CH5n/Fo9wN/MWMSUN5WIZPiCAqmMECWIJ0wEFrD70SXxItc40lTY
Ej7Cg9HwldlZhJ3gerlMN8MTZl+bmi2z0c0fF2vUgSa4X76s2h7xM/uKFS7VeDbO
NcIFITUWBfyhkvjfJrSJdR81sWJQEjsoDVA2YwgmfkIBTkAGOOfiEP0Lt1deZ1+O
vR9Dlt5lETscY76CFToOD6KlGRvZ+MKuP9Jvp8NljrOxCDevayxrWq/Vl5pwJbWs
WCoQmAUplVB08aeXjw0V/ZklXJTa51THQ40O1QMSSC17EQgd/kh0D4PqAhjUJ+M2
S/gyDm2PubZOJRwXdPT9Mv+q4emY5Dn/MiY3ZMBbMeqXytMe9J+3YY/EWkcVwgFU
LcbwxNKz7fqTtZkvNibuYxAjwPFqbh0/m2QxtqMtZs9aXHPT2Lno2HGcmLoaIUH/
RgCQjQjS7h+tC2NOc9OPNW+lfHFctHmkuv2GkhbGD2v0hamxJuMx8NxXVNYoClSR
X9PiT5VxoZtNW/SyX9ff
=QWYQ
-----END PGP SIGNATURE-----
Tom Rini March 10, 2014, 5:52 p.m. UTC | #5 
On Mon, Mar 10, 2014 at 01:28:31PM -0400, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 03/10/2014 01:24 PM, Stephen Warren wrote:
> > On 03/05/2014 09:45 AM, Przemyslaw Marczak wrote:
> >> 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 <swarren@nvidia.com>
> 
> As I'm applying and testing this locally right now, I'll take that
> better wording (and ack), thanks!
> 
> >> diff --git a/lib/Makefile b/lib/Makefile
> > 
> >>  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.

On second thought, and reading the rest of Stephen's review, please make
a v3 of the series incorporating his feedback, making sure that the
series is bisectable, we drop the unused str_ptr from 1/3 and also we
catch the other direct use of '36' rather than UUID_STR_LEN in
lib/uuid.c.  Note that to maintain bisectablity I'm OK with adding '36'
directly in one patch and removing in the next.
diff mbox

Patch

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 96a45a6..32377ad 100644
--- a/include/common.h
+++ b/include/common.h
@@ -815,7 +815,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 dedb97b..70962b2 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_RANDOM_MACADDR) += rand.o
 obj-$(CONFIG_BOOTP_RANDOM_DELAY) += rand.o
diff --git a/lib/uuid.c b/lib/uuid.c
index c48bf38..803bdcd 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -5,7 +5,8 @@ 
  */
 
 #include <linux/ctype.h>
-#include "common.h"
+#include <errno.h>
+#include <common.h>
 
 /*
  * This is what a UUID string looks like.
@@ -43,14 +44,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 +70,27 @@  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};
+	char *str_ptr = str;
+	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;
+		}
+	}
 }