| Message ID | 1496076573-14495-5-git-send-email-alison@peloton-tech.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
alison@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
Hi Alison, > 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. Acked-by: Lukasz Majewski <lukma@denx.de> > > 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)) { > + 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)); > + memset(newpart, '\0', sizeof(newpart)); > + if (!newpart) > + 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); > + strncpy((char *)newpart->gpt_part_info.type, (const char > *)info->type, PART_TYPE_LEN); > + memset(newpart->gpt_part_info.type + 31, '\0', 1); > + 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 */ > + int valid_parts = 1, p, ret = 0; > + 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)) > + return -1; > + > + 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; > +} > + > + > /** > * 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); > } 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 > > typedef struct disk_partition { > lbaint_t start; /* # of first block in > partition */ @@ -68,6 +70,12 @@ typedef struct disk_partition { > #endif > } disk_partition_t; > > +struct disk_part { > + int partnum; > + disk_partition_t gpt_part_info; > + struct list_head list; > +}; > + > /* Misc _get_dev functions */ > #ifdef CONFIG_PARTITIONS > /** 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:48:45 +0200 Lukasz Majewski wrote: > Hi Alison, > > > 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. > > Acked-by: Lukasz Majewski <lukma@denx.de> > You definitely should read other peoples comments on patches before blindly Acking them! Lothar Waßmann
On Wed, 31 May 2017 10:48:59 +0200 Lothar Waßmann <LW@KARO-electronics.de> wrote: > Hi, > > On Wed, 31 May 2017 09:48:45 +0200 Lukasz Majewski wrote: > > Hi Alison, > > > > > 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. > > > > Acked-by: Lukasz Majewski <lukma@denx.de> > > > You definitely should read other peoples comments on patches before > blindly Acking them! Your reply to the original thread (v1): From: Lothar Waßmann <LW@KARO-electronics.de> To: alison@peloton-tech.com Cc: u-boot@lists.denx.de, alison@she-devel.com, trini@konsulko.com I was not added to TO/CC. And unfortunately (from the lack of time) - I don't have time to promptly reply/see all the responses to the ML. Alison sent v2 on Mon (to which I was added to CC). I've assumed that this was fixed for v2. Wasn't it? > > > Lothar Waßmann 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
Lukasz Majewski <lukma@denx.de> wrote: > On Wed, 31 May 2017 10:48:59 +0200 > Lothar Waßmann <LW@KARO-electronics.de> wrote: > > > Hi, > > > > On Wed, 31 May 2017 09:48:45 +0200 Lukasz Majewski wrote: > > > Hi Alison, > > > > > > > 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. > > > > > > Acked-by: Lukasz Majewski <lukma@denx.de> > > > > > You definitely should read other peoples comments on patches before > > blindly Acking them! > > Your reply to the original thread (v1): > > From: Lothar Waßmann <LW@KARO-electronics.de> > To: alison@peloton-tech.com > Cc: u-boot@lists.denx.de, alison@she-devel.com, trini@konsulko.com > > I was not added to TO/CC. > > And unfortunately (from the lack of time) - I don't have time to > promptly reply/see all the responses to the ML. > > > Alison sent v2 on Mon (to which I was added to CC). > > I've assumed that this was fixed for v2. Wasn't it? > I replied to his v2 patch. Lothar Waßmann
Hi Alison, > Hi Alison, > > > 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. > > Acked-by: Lukasz Majewski <lukma@denx.de> Sorry, but I was to fast. Please fix problems pointed out by Lothar. > > > > > 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> > > +Lothar Waßmann <LW@KARO-electronics.de> > > +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)) { > > + 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)); > > + memset(newpart, '\0', sizeof(newpart)); > > + if (!newpart) > > + 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); > > + strncpy((char *)newpart->gpt_part_info.type, (const char > > *)info->type, PART_TYPE_LEN); > > + memset(newpart->gpt_part_info.type + 31, '\0', 1); > > + 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 */ > > + int valid_parts = 1, p, ret = 0; > > + 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)) > > + return -1; > > + > > + 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; > > +} > > + > > + > > /** > > * 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); > > } 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 > > > > typedef struct disk_partition { > > lbaint_t start; /* # of first block in > > partition */ @@ -68,6 +70,12 @@ typedef struct > > disk_partition { #endif > > } disk_partition_t; > > > > +struct disk_part { > > + int partnum; > > + disk_partition_t gpt_part_info; > > + struct list_head list; > > +}; > > + > > /* Misc _get_dev functions */ > > #ifdef CONFIG_PARTITIONS > > /** > > > > > 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 > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot 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
On 2017-05-30 00:37, Lothar Waßmann wrote: [ SNIP] >> +#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, I'm working on making the other changes you suggested. Meanwhile, the answer to this question is that I copied the macro from a header file in 2015.07, which is the version for which I developed the patches. I saw no equivalent value in this latest release of Das U-Boot. Do you have a better suggestion? Is there a similar macro elsewhere that I failed to discover? Thanks, Alison --- Alison Chaiken alison@she-devel.com, 650-279-5600 http://{ she-devel.com, exerciseforthereader.org } "We are giving up our privacy, one convenience at a time." -- Evangelos Simoudis
"Chaiken, Alison" <alison@she-devel.com> wrote: > On 2017-05-30 00:37, Lothar Waßmann wrote: > [ SNIP] > >> +#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, I'm working on making the other changes you suggested. > Meanwhile, the answer to this question is that I copied the macro from a > header file in 2015.07, which is the version for which I developed the > patches. I saw no equivalent value in this latest release of Das > U-Boot. Do you have a better suggestion? Is there a similar macro > elsewhere that I failed to discover? > I'm just concerned, that the limit may be too low for a certain class of systems and see no reason why it shouldn't be set higher. There is no memory allocation involved that would consume more memory with a higher limit. I'v seen, the macro is defined in disk/part.c. Maybe for consistency this should be defined in some header file and used in the original place and your patch. Lothar Waßmann
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)) { + 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)); + memset(newpart, '\0', sizeof(newpart)); + if (!newpart) + 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); + strncpy((char *)newpart->gpt_part_info.type, (const char *)info->type, PART_TYPE_LEN); + memset(newpart->gpt_part_info.type + 31, '\0', 1); + 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 */ + int valid_parts = 1, p, ret = 0; + 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)) + return -1; + + 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; +} + + /** * 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); } 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 typedef struct disk_partition { lbaint_t start; /* # of first block in partition */ @@ -68,6 +70,12 @@ typedef struct disk_partition { #endif } disk_partition_t; +struct disk_part { + int partnum; + disk_partition_t gpt_part_info; + struct list_head list; +}; + /* Misc _get_dev functions */ #ifdef CONFIG_PARTITIONS /**