[U-Boot] [PATCH v2 4/6] GPT: read partition table from device into a data structure

["Lothar Waßmann" <LW@KARO-electronics.de>]

alison at peloton-tech.com wrote:

> From: Alison Chaiken <alison@she-devel.com>
> 
> Make the partition table available for modification by reading it from
> the user-specified device into a linked list.   Provide an accessor
> function for command-line testing.
> 
> Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
> ---
>  cmd/gpt.c      | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/part.h |   8 +++++
>  2 files changed, 120 insertions(+)
> 
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index 3b7d929..c61d2b1 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -19,6 +19,9 @@
>  #include <linux/ctype.h>
>  #include <div64.h>
>  #include <memalign.h>
> +#include <linux/compat.h>
> +
> +static LIST_HEAD(disk_partitions);
>  
>  /**
>   * extract_env(): Expand env name from string format '&{env_name}'
> @@ -151,6 +154,111 @@ static bool found_key(const char *str, const char *key)
>  	return result;
>  }
>  
> +static void del_gpt_info(void)
> +{
> +	struct list_head *pos = &disk_partitions;
> +	struct disk_part *curr;
> +	while (!list_empty(pos)) {
>
empty line between declarations and code?

> +		curr = list_entry(pos->next, struct disk_part, list);
> +		list_del(pos->next);
> +		free(curr);
> +	}
> +}
> +
> +static struct disk_part *allocate_disk_part(disk_partition_t *info, int partnum)
> +{
> +	struct disk_part *newpart;
> +	newpart = (struct disk_part *)malloc(sizeof(*newpart));
>
useless type cast. malloc returns a void pointer which can be assigned
to any typed pointer without a cast.

> +	memset(newpart, '\0', sizeof(newpart));
> +	if (!newpart)
>
This NULL check should be done before the memset()!

> +		return ERR_PTR(-ENOMEM);
> +
> +	newpart->gpt_part_info.start = info->start;
> +	newpart->gpt_part_info.size = info->size;
> +	newpart->gpt_part_info.blksz = info->blksz;
> +	strncpy((char *)newpart->gpt_part_info.name, (const char *)info->name, PART_NAME_LEN);
> +	memset(newpart->gpt_part_info.name + 31, '\0', 1);
	newpart->gpt_part_info.name[PART_NAME_LEN - 1] = '\0';
1. No need for memset here.
2. You are copying PART_NAME_LEN bytes, but set the byte at pos 31 to
   '\0'. => This code will blow up if PART_NAME_LEN is changed to a
different value!

> +	strncpy((char *)newpart->gpt_part_info.type, (const char *)info->type, PART_TYPE_LEN);
> +	memset(newpart->gpt_part_info.type + 31, '\0', 1);
dto.

> +	newpart->gpt_part_info.bootable = info->bootable;
> +#ifdef CONFIG_PARTITION_UUIDS
> +	strncpy(newpart->gpt_part_info.uuid, (const char *)info->uuid,
> +		UUID_STR_LEN);
> +#endif
> +	newpart->partnum = partnum;
> +
> +	return newpart;
> +}
> +
> +static void print_gpt_info(void)
> +{
> +	struct list_head *pos;
> +	struct disk_part *curr;
> +
> +	list_for_each(pos, &disk_partitions) {
> +		curr = list_entry(pos, struct disk_part, list);
> +		printf("Partition %d:\n", curr->partnum);
> +		printf("1st block %x, size %x\n", (unsigned)curr->gpt_part_info.start,
> +		       (unsigned)curr->gpt_part_info.size);
> +		printf("Block size %lu, name %s\n", curr->gpt_part_info.blksz,
> +		       curr->gpt_part_info.name);
> +		printf("Type %s, bootable %d\n", curr->gpt_part_info.type,
> +		       curr->gpt_part_info.bootable);
> +#ifdef CONFIG_PARTITION_UUIDS
> +		printf("UUID %s\n", curr->gpt_part_info.uuid);
> +#endif
> +		printf("\n");
> +	}
> +}
> +
> +/*
> + * read partition info into disk_partitions list where
> + * it can be printed or modified
> + */
> +static int get_gpt_info(struct blk_desc *dev_desc)
> +{
> +	/* start partition numbering at 1, as u-boot does */
>
s/u-boot/U-Boot/

> +	int valid_parts = 1, p, ret = 0;
>
No need to initialize ret here (no need to declare it here either).

> +	disk_partition_t info;
> +	struct disk_part *new_disk_part;
> +
> +	if (disk_partitions.next == NULL)
> +		INIT_LIST_HEAD(&disk_partitions);
> +
> +	for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +		ret = part_get_info(dev_desc, p, &info);
> +		if (ret)
> +			continue;
> +
> +		new_disk_part = allocate_disk_part(&info, valid_parts);
> +		if (IS_ERR(new_disk_part) && (valid_parts >= 2))
>
No need for () around the '>=' expression.

> +			return -1;
You return -ENODEV lateron, so you should return a valid errno value
here. -1 (-EPERM) is most probably not the error code you want to use
here.

> +		list_add_tail(&new_disk_part->list, &disk_partitions);
> +		valid_parts++;
> +	}
> +	if (!valid_parts) {
> +		printf("** No valid partitions found **\n");
> +		del_gpt_info();
> +		return -ENODEV;
> +	}
> +	return --valid_parts;
> +}
> +
> +/* a wrapper to test get_gpt_info */
> +static int do_get_gpt_info(struct blk_desc *dev_desc)
> +{
> +	int ret;
> +
> +	ret = get_gpt_info(dev_desc);
> +	if (ret > 0) {
> +		print_gpt_info();
> +		del_gpt_info();
> +	}
> +	return ret;
>
Since the return value of this function is passed on as the exit code
of the calling CMD handler it should be one of CMD_RET_SUCCESS or
CMD_RET_FAILURE:
	return ret ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
But see my comment below!

>  /**
>   * set_gpt_info(): Fill partition information from string
>   *		function allocates memory, remember to free!
> @@ -457,6 +565,8 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		if (argc == 5)
>  		       strcpy(varname, argv[4]);
>  		return do_disk_guid(blk_dev_desc, varname);
> +	} else if (strcmp(argv[1], "read") == 0) {
> +		return do_get_gpt_info(blk_dev_desc);
>
If you have a careful look at the succeeding code in this function you
would see, that it does not blindly pass on the return value of the
functions called for each subcommand but does:
|	if (ret) {
|		printf("error!\n");
|		return CMD_RET_FAILURE;
|	}
|
|	printf("success!\n");
|	return CMD_RET_SUCCESS;

So you should just assign the return value of do_disk_guid() and
do_get_gpt_info() to the variable 'ret' and let the original code
handle it.

>  	} else {
>  		return CMD_RET_USAGE;
>  	}
> @@ -479,6 +589,8 @@ 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"
> +	" read <interface> <dev>\n"
> +	"    - read GPT into a data structure for manipulation\n"
>  	" guid <interface> <dev>\n"
>  	"    - print disk GUID\n"
>  	" guid <interface> <dev> <varname>\n"
> diff --git a/include/part.h b/include/part.h
> index 16c4a46..7d7052a 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -10,6 +10,7 @@
>  #include <blk.h>
>  #include <ide.h>
>  #include <uuid.h>
> +#include <linux/list.h>
>  
>  struct block_drvr {
>  	char *name;
> @@ -49,6 +50,7 @@ struct block_drvr {
>  
>  #define PART_NAME_LEN 32
>  #define PART_TYPE_LEN 32
> +#define MAX_SEARCH_PARTITIONS 16
>  
Why the hard limit to 16 partitions?
A standard Android system will jump right in your face with this limit.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________