[U-Boot,v2,3/6] GPT: add accessor function for disk GUID

From: Alison Chaiken <alison@she-devel.com>

In order to read the GPT, modify the partition name strings, and then
write out a new GPT, the disk GUID is needed.  While there is an
existing accessor for the partition UUIDs, there is none yet for the
disk GUID.

Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
---
 cmd/gpt.c       | 30 +++++++++++++++++++++++++++++-
 disk/part_efi.c | 31 +++++++++++++++++++++++++++++++
 doc/README.gpt  |  3 ++-
 include/part.h  | 15 +++++++++++++++
 4 files changed, 77 insertions(+), 2 deletions(-)

alison@peloton-tech.com wrote:

> From: Alison Chaiken <alison@she-devel.com>
> 
> In order to read the GPT, modify the partition name strings, and then
> write out a new GPT, the disk GUID is needed.  While there is an
> existing accessor for the partition UUIDs, there is none yet for the
> disk GUID.
> 
> Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
> ---
>  cmd/gpt.c       | 30 +++++++++++++++++++++++++++++-
>  disk/part_efi.c | 31 +++++++++++++++++++++++++++++++
>  doc/README.gpt  |  3 ++-
>  include/part.h  | 15 +++++++++++++++
>  4 files changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index 3e98821..3b7d929 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -398,6 +398,23 @@ static int gpt_verify(struct blk_desc *blk_dev_desc, const char *str_part)
>  	return ret;
>  }
>  
> +static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr)
> +{
> +	int ret;
> +	char disk_guid[UUID_STR_LEN + 1];
> +
> +	ret = get_disk_guid(dev_desc, disk_guid);
> +	if (ret < 0)
> +		return 1;
> +
Since you are calling this function from a CMD handler and passing its
return value as the CMD handlers exit code this should be:
		return CMD_RET_FAILURE;

> +	if (namestr)
> +		setenv(namestr, disk_guid);
> +	else
> +		printf("%s\n", disk_guid);
> +
> +	return 0;
>
... and this:
	return CMD_RET_SUCCESS;

>  /**
>   * do_gpt(): Perform GPT operations
>   *
> @@ -412,7 +429,7 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>  	int ret = CMD_RET_SUCCESS;
>  	int dev = 0;
> -	char *ep;
> +	char *ep, *varname = NULL;
>  	struct blk_desc *blk_dev_desc = NULL;
>  
>  	if (argc < 4 || argc > 5)
> @@ -436,6 +453,10 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	} else if ((strcmp(argv[1], "verify") == 0)) {
>  		ret = gpt_verify(blk_dev_desc, argv[4]);
>  		printf("Verify GPT: ");
> +	} else if (strcmp(argv[1], "guid") == 0) {
> +		if (argc == 5)
> +		       strcpy(varname, argv[4]);
>
You didn't allocate any memory for varname and copy the argument string
to a NULL pointer!
Maybe use strdup() and free varname after use?

> +		return do_disk_guid(blk_dev_desc, varname);
>  	} else {
>  		return CMD_RET_USAGE;
>  	}
> @@ -458,4 +479,11 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
>  	" Example usage:\n"
>  	" gpt write mmc 0 $partitions\n"
>  	" gpt verify mmc 0 $partitions\n"
> +	" guid <interface> <dev>\n"
> +	"    - print disk GUID\n"
> +	" guid <interface> <dev> <varname>\n"
> +	"    - set environment variable to disk GUID\n"
> +	" Example usage:\n"
> +	" gpt guid mmc 0\n"
> +	" gpt guid mmc 0 varname\n"
>  );
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 20d33ef..ff9f408 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -178,6 +178,37 @@ static void prepare_backup_gpt_header(gpt_header *gpt_h)
>   * Public Functions (include/part.h)
>   */
>  
> +/*
> + * UUID is displayed as 32 hexadecimal digits, in 5 groups,
> + * separated by hyphens, in the form 8-4-4-4-12 for a total of 36 characters
> + */
> +int get_disk_guid(struct blk_desc * dev_desc, char *guid)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, dev_desc->blksz);
> +	gpt_entry *gpt_pte = NULL;
> +	unsigned char *guid_bin;
> +
> +	/* This function validates AND fills in the GPT header and PTE */
> +	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
> +			 gpt_head, &gpt_pte) != 1) {
> +		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
> +		if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
>
useless '()'

> +				 gpt_head, &gpt_pte) != 1) {
> +			printf("%s: *** ERROR: Invalid Backup GPT ***\n",
> +			       __func__);
> +			return -1;
	return -EINVAL;?

> +		} else {
> +			printf("%s: ***        Using Backup GPT ***\n",
> +			       __func__);
> +		}
> +	}
> +
> +	guid_bin = (unsigned char *)(gpt_head->disk_guid.b);
>
disk_guid.b is of type u8 which is the same as unsigned char.
No need for a type cast here (and the () around gpt_head->... are
unnecessary).

> +	uuid_bin_to_str(guid_bin, guid, UUID_STR_FORMAT_GUID);
> +
> +	return 0;
> +}
> +
>  void part_print_efi(struct blk_desc *dev_desc)
>  {
>  	ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, dev_desc->blksz);
> diff --git a/doc/README.gpt b/doc/README.gpt
> index 3fcd835..c0779a4 100644
> --- a/doc/README.gpt
> +++ b/doc/README.gpt
> @@ -171,7 +171,8 @@ To restore GUID partition table one needs to:
>     The fields 'uuid' and 'uuid_disk' are optional if CONFIG_RANDOM_UUID is
>     enabled. A random uuid will be used if omitted or they point to an empty/
>     non-existent environment variable. The environment variable will be set to
> -   the generated UUID.
> +   the generated UUID.  The 'gpt guid' command reads the current value of the
> +   uuid_disk from the GPT.
>  
>     The field 'bootable' is optional, it is used to mark the GPT partition
>     bootable (set attribute flags "Legacy BIOS bootable").
> diff --git a/include/part.h b/include/part.h
> index 87b1111..16c4a46 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -371,6 +371,21 @@ int gpt_verify_headers(struct blk_desc *dev_desc, gpt_header *gpt_head,
>  int gpt_verify_partitions(struct blk_desc *dev_desc,
>  			  disk_partition_t *partitions, int parts,
>  			  gpt_header *gpt_head, gpt_entry **gpt_pte);
> +
> +
> +/**
> + * get_disk_guid() - Function to read the GUID string from a device's GPT
> + *
> + * This function reads the GUID string from a block device whose descriptor
> + * is provided.
> + *
> + * @param dev_desc - block device descriptor
> + * @param guid - pre-allocated string in which to return the GUID
> + *
> + * @return - '0' on success, otherwise error
> + */
> +int get_disk_guid(struct blk_desc *dev_desc, char *guid);
> +
>  #endif
>  
>  #if CONFIG_IS_ENABLED(DOS_PARTITION)

Hi Alison,

> From: Alison Chaiken <alison@she-devel.com>
> 
> In order to read the GPT, modify the partition name strings, and then
> write out a new GPT, the disk GUID is needed.  While there is an
> existing accessor for the partition UUIDs, there is none yet for the
> disk GUID.

Acked-by: Lukasz Majewski <lukma@denx.de>

> 
> Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
> ---
>  cmd/gpt.c       | 30 +++++++++++++++++++++++++++++-
>  disk/part_efi.c | 31 +++++++++++++++++++++++++++++++
>  doc/README.gpt  |  3 ++-
>  include/part.h  | 15 +++++++++++++++
>  4 files changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index 3e98821..3b7d929 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -398,6 +398,23 @@ static int gpt_verify(struct blk_desc
> *blk_dev_desc, const char *str_part) return ret;
>  }
>  
> +static int do_disk_guid(struct blk_desc *dev_desc, char * const
> namestr) +{
> +	int ret;
> +	char disk_guid[UUID_STR_LEN + 1];
> +
> +	ret = get_disk_guid(dev_desc, disk_guid);
> +	if (ret < 0)
> +		return 1;
> +
> +	if (namestr)
> +		setenv(namestr, disk_guid);
> +	else
> +		printf("%s\n", disk_guid);
> +
> +	return 0;
> +}
> +
>  /**
>   * do_gpt(): Perform GPT operations
>   *
> @@ -412,7 +429,7 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) {
>  	int ret = CMD_RET_SUCCESS;
>  	int dev = 0;
> -	char *ep;
> +	char *ep, *varname = NULL;
>  	struct blk_desc *blk_dev_desc = NULL;
>  
>  	if (argc < 4 || argc > 5)
> @@ -436,6 +453,10 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag,
> int argc, char * const argv[]) } else if ((strcmp(argv[1], "verify")
> == 0)) { ret = gpt_verify(blk_dev_desc, argv[4]);
>  		printf("Verify GPT: ");
> +	} else if (strcmp(argv[1], "guid") == 0) {
> +		if (argc == 5)
> +		       strcpy(varname, argv[4]);
> +		return do_disk_guid(blk_dev_desc, varname);
>  	} else {
>  		return CMD_RET_USAGE;
>  	}
> @@ -458,4 +479,11 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
>  	" Example usage:\n"
>  	" gpt write mmc 0 $partitions\n"
>  	" gpt verify mmc 0 $partitions\n"
> +	" guid <interface> <dev>\n"
> +	"    - print disk GUID\n"
> +	" guid <interface> <dev> <varname>\n"
> +	"    - set environment variable to disk GUID\n"
> +	" Example usage:\n"
> +	" gpt guid mmc 0\n"
> +	" gpt guid mmc 0 varname\n"
>  );
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 20d33ef..ff9f408 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -178,6 +178,37 @@ static void prepare_backup_gpt_header(gpt_header
> *gpt_h)
>   * Public Functions (include/part.h)
>   */
>  
> +/*
> + * UUID is displayed as 32 hexadecimal digits, in 5 groups,
> + * separated by hyphens, in the form 8-4-4-4-12 for a total of 36
> characters
> + */
> +int get_disk_guid(struct blk_desc * dev_desc, char *guid)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1,
> dev_desc->blksz);
> +	gpt_entry *gpt_pte = NULL;
> +	unsigned char *guid_bin;
> +
> +	/* This function validates AND fills in the GPT header and
> PTE */
> +	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
> +			 gpt_head, &gpt_pte) != 1) {
> +		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
> +		if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
> +				 gpt_head, &gpt_pte) != 1) {
> +			printf("%s: *** ERROR: Invalid Backup GPT
> ***\n",
> +			       __func__);
> +			return -1;
> +		} else {
> +			printf("%s: ***        Using Backup GPT
> ***\n",
> +			       __func__);
> +		}
> +	}
> +
> +	guid_bin = (unsigned char *)(gpt_head->disk_guid.b);
> +	uuid_bin_to_str(guid_bin, guid, UUID_STR_FORMAT_GUID);
> +
> +	return 0;
> +}
> +
>  void part_print_efi(struct blk_desc *dev_desc)
>  {
>  	ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1,
> dev_desc->blksz); diff --git a/doc/README.gpt b/doc/README.gpt
> index 3fcd835..c0779a4 100644
> --- a/doc/README.gpt
> +++ b/doc/README.gpt
> @@ -171,7 +171,8 @@ To restore GUID partition table one needs to:
>     The fields 'uuid' and 'uuid_disk' are optional if
> CONFIG_RANDOM_UUID is enabled. A random uuid will be used if omitted
> or they point to an empty/ non-existent environment variable. The
> environment variable will be set to
> -   the generated UUID.
> +   the generated UUID.  The 'gpt guid' command reads the current
> value of the
> +   uuid_disk from the GPT.
>  
>     The field 'bootable' is optional, it is used to mark the GPT
> partition bootable (set attribute flags "Legacy BIOS bootable").
> diff --git a/include/part.h b/include/part.h
> index 87b1111..16c4a46 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -371,6 +371,21 @@ int gpt_verify_headers(struct blk_desc
> *dev_desc, gpt_header *gpt_head, int gpt_verify_partitions(struct
> blk_desc *dev_desc, disk_partition_t *partitions, int parts,
>  			  gpt_header *gpt_head, gpt_entry **gpt_pte);
> +
> +
> +/**
> + * get_disk_guid() - Function to read the GUID string from a
> device's GPT
> + *
> + * This function reads the GUID string from a block device whose
> descriptor
> + * is provided.
> + *
> + * @param dev_desc - block device descriptor
> + * @param guid - pre-allocated string in which to return the GUID
> + *
> + * @return - '0' on success, otherwise error
> + */
> +int get_disk_guid(struct blk_desc *dev_desc, char *guid);
> +
>  #endif
>  
>  #if CONFIG_IS_ENABLED(DOS_PARTITION)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Hi,

On Wed, 31 May 2017 09:44:43 +0200 Lukasz Majewski wrote:
> Hi Alison,
> 
> > From: Alison Chaiken <alison@she-devel.com>
> > 
> > In order to read the GPT, modify the partition name strings, and then
> > write out a new GPT, the disk GUID is needed.  While there is an
> > existing accessor for the partition UUIDs, there is none yet for the
> > disk GUID.
> 
> Acked-by: Lukasz Majewski <lukma@denx.de>
> 
Did you read my comments on this patch in:
<20170530084651.545d9954@ipc1.ka-ro>
e.g.:
|> +		if (argc == 5)
|> +		       strcpy(varname, argv[4]);
|>  
|You didn't allocate any memory for varname and copy the argument string
|to a NULL pointer!
|Maybe use strdup() and free varname after use?



Lothar Waßmann

From: Alison Chaiken <alison@peloton-tech.com>

One way for userspace and the bootloader to exchange information about
dynamic image selection is via the storage device partition table, as
described at

https://source.android.com/devices/tech/ota/ab_updates

The scheme described there relies on setting partitions' "boot" flag.
When no partition on a device is bootable since the kernel and U-Boot
are stored elsewhere, the name field in the GPT partition table offers
another logical place to store information.  These patches allow users
to easily modify GPT partition names via bootscripts that can select
different images based on a boot-failure counter, or when userspace
installs a software update.

These patches have been (re)tested on a TI DRA7xx-based SOM with
U-Boot 2015.07.  The storage device is an eMMC.

Significant changes since v2:
-- Got rid of the need to allocate memory for the GUID string in
   do_gpt();
-- Fixed the problems with string NULL termination in
   allocate_disk_part();
-- Removed duplicate definition of MAX_SEARCH_PARTITIONS from
   disk/part.c and increased the value in include/part.h to 64;
-- Improved the commit message for "rename GPT partitions to detect
   boot failure" to better describe the version of the patch I
   submitted;
-- Fixed numerous small problems with function return values.

Significant changes since v1:
-- Put the gpt_flip() function and auxiliary ones inside a new
   CONFIG_CMD_GPT_FLIP option that depends on CMD_GPT.
-- Replace intentional overwriting of name and type string arrays with
   memset() instead.
-- Move part.h changes earlier in the patchset.
-- Add a few lines to README.gpt about the new gpt subcommands.

Added a few simple patches to do the following:
-- Replace remaining occurrences of '37' with UUID_STR_LEN+1;
-- Introduce new macros to get rid of the similar '32';
-- fix a smaller error in doc/README.gpt.

There are also some fixups of whitespace and formatting errors (plus
usual inevitable addition of new ones).

To do in future:
-- Add support for preserving the type flag for partitions. The u-boot
   version on which this patchset is based did not have this feature,
   and it's easy to add, but I need to figure how to test it first.

-- Add tests for the new gpt commands to the sandbox.


Alison Chaiken (5):
  GPT: add accessor function for disk GUID
  partitions: increase MAX_SEARCH_PARTITIONS and move to part.h
  GPT: read partition table from device into a data structure
  rename GPT partitions to detect boot failure
  GPT: fix error in partitions string doc

 cmd/Kconfig     |   7 ++
 cmd/gpt.c       | 318 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 disk/part.c     |   1 -
 disk/part_efi.c |  31 ++++++
 doc/README.gpt  |  24 ++++-
 include/part.h  |  23 ++++
 6 files changed, 398 insertions(+), 6 deletions(-)

On Fri,  2 Jun 2017 19:22:29 -0700
alison@peloton-tech.com wrote:

> Significant changes since v2:
> -- Got rid of the need to allocate memory for the GUID string in
>    do_gpt();
> -- Fixed the problems with string NULL termination in
>    allocate_disk_part();
> -- Removed duplicate definition of MAX_SEARCH_PARTITIONS from
>    disk/part.c and increased the value in include/part.h to 64;
> -- Improved the commit message for "rename GPT partitions to detect
>    boot failure" to better describe the version of the patch I
>    submitted;
> -- Fixed numerous small problems with function return values.

Just informative - (no need to resend patches) - for next submissions -
please add changelog to each relevant patch.

In this way one can quickly refer to the code.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de