[PATCH 0/3] cmd: fix errors in gpt command
Consider that partitions may not be numbered continously starting at 1. Don't enumerate a block device with multiple partition drivers. Let gpt_partition_entry be a hexadecimal value.
Heinrich Schuchardt (3): cmd: fix gpt setenv cmd: fix gpt enumerate cmd: let gpt_partition_entry be hexadecimal
cmd/gpt.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
Do not assume that partitions are continuously numbered starting at 1.
Having a partition table with a single partition 63 is valid.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- cmd/gpt.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/cmd/gpt.c b/cmd/gpt.c index 007a68eaa7..d0e165d539 100644 --- a/cmd/gpt.c +++ b/cmd/gpt.c @@ -786,10 +786,8 @@ static int gpt_setenv(struct blk_desc *desc, const char *name)
for (i = 1; i < part_drv->max_entries; i++) { ret = part_drv->get_info(desc, i, &pinfo); - if (ret) { - /* no more entries in table */ - break; - } + if (ret) + continue;
if (!strcmp(name, (const char *)pinfo.name)) { /* match found, setup environment variables */
On Fri, 25 Aug 2023 at 19:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Do not assume that partitions are continuously numbered starting at 1.
Having a partition table with a single partition 63 is valid.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/gpt.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
I would like to have a map / list of the partition on a disk for bootstd. At present we iterate through a lot of partitions that are not present.
What do you think?
Regards, Simon
On 26.08.23 04:04, Simon Glass wrote:
On Fri, 25 Aug 2023 at 19:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Do not assume that partitions are continuously numbered starting at 1.
Having a partition table with a single partition 63 is valid.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/gpt.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
I would like to have a map / list of the partition on a disk for bootstd. At present we iterate through a lot of partitions that are not present.
What do you think?
Regards, Simon
Reading the partition table and buffering it is possible in principle. This is what Linux does. But you have to remember that there are multiple commands that can overwrite the partition table, e.g.
* gpt write * mbr write * mmc write * nvme write * sata write
Or there could be a user action like swapping the SD card.
So you will need an invalidation logic for the buffer.
How much boot time could you save on an embedded board?
Best regards
Heinrich
Do not assume that partitions are numbered continuously starting at 1.
Only a single partition table type can exist on a block device. If we found a GPT partition table, we must not re-enumerate with the MBR partition driver which would find the protective partition.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- cmd/gpt.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/cmd/gpt.c b/cmd/gpt.c index d0e165d539..99ca0a6163 100644 --- a/cmd/gpt.c +++ b/cmd/gpt.c @@ -691,12 +691,13 @@ static int gpt_enumerate(struct blk_desc *desc) int ret; int i;
+ if (part_drv->test(desc)) + continue; + for (i = 1; i < part_drv->max_entries; i++) { ret = part_drv->get_info(desc, i, &pinfo); - if (ret) { - /* no more entries in table */ - break; - } + if (ret) + continue;
ptr = &part_list[str_len]; tmp_len = strlen((const char *)pinfo.name); @@ -711,9 +712,10 @@ static int gpt_enumerate(struct blk_desc *desc) /* One byte for space(" ") delimiter */ ptr[tmp_len] = ' '; } + if (*part_list) + part_list[strlen(part_list) - 1] = 0; + break; } - if (*part_list) - part_list[strlen(part_list) - 1] = 0; debug("setenv gpt_partition_list %s\n", part_list);
return env_set("gpt_partition_list", part_list);
Hi Heinrich,
On Fri, 25 Aug 2023 at 19:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Do not assume that partitions are numbered continuously starting at 1.
Only a single partition table type can exist on a block device. If we found a GPT partition table, we must not re-enumerate with the MBR partition driver which would find the protective partition.
Does it really? My understand is that it sets desc->part_type and it stays like that from then on?
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/gpt.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/cmd/gpt.c b/cmd/gpt.c index d0e165d539..99ca0a6163 100644 --- a/cmd/gpt.c +++ b/cmd/gpt.c @@ -691,12 +691,13 @@ static int gpt_enumerate(struct blk_desc *desc) int ret; int i;
if (part_drv->test(desc))continue;for (i = 1; i < part_drv->max_entries; i++) { ret = part_drv->get_info(desc, i, &pinfo);
if (ret) {/* no more entries in table */break;}
if (ret)continue; ptr = &part_list[str_len]; tmp_len = strlen((const char *)pinfo.name);@@ -711,9 +712,10 @@ static int gpt_enumerate(struct blk_desc *desc) /* One byte for space(" ") delimiter */ ptr[tmp_len] = ' '; }
if (*part_list)part_list[strlen(part_list) - 1] = 0;break; }
if (*part_list)part_list[strlen(part_list) - 1] = 0; debug("setenv gpt_partition_list %s\n", part_list); return env_set("gpt_partition_list", part_list);-- 2.40.1
On 8/26/23 04:04, Simon Glass wrote:
Hi Heinrich,
On Fri, 25 Aug 2023 at 19:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Do not assume that partitions are numbered continuously starting at 1.
Only a single partition table type can exist on a block device. If we found a GPT partition table, we must not re-enumerate with the MBR partition driver which would find the protective partition.
Does it really? My understand is that it sets desc->part_type and it stays like that from then on?
Prior to my patch with printf statements added:
=> gpt enumerate host 0 enumeration by driver EFI partition 'EFI system partition' enumeration by driver AMIGA enumeration by driver DOS partition 'xxa1' enumeration by driver ISO enumeration by driver MAC setenv gpt_partition_list EFI system partition xxa1 success!
Of course the value of $gpt_partition_list is useless because you do not even know how many partitions were discovered 'EFI', 'system', 'partition' could be the names of three different partitions or there could be one partition 'EFI system partition'.
I wonder how Broadcom can use this in their update scripts. See 12fc1f3bb223 ("cmd: gpt: add eMMC and GPT support").
For something usable we would have to add quotations marks, escape quotation marks in the string, and the do-for command would need to support a string with quotation marks like
"'Heinrich's partition one' 'EFI system partition'"
The "xxa1" string is generated in part_set_generic_name() because MBR partitions don't have a name.
xx = other block device type a = device number 0 1 = partition number 0
Best regards
Heinrich
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/gpt.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/cmd/gpt.c b/cmd/gpt.c index d0e165d539..99ca0a6163 100644 --- a/cmd/gpt.c +++ b/cmd/gpt.c @@ -691,12 +691,13 @@ static int gpt_enumerate(struct blk_desc *desc) int ret; int i;
if (part_drv->test(desc))continue;for (i = 1; i < part_drv->max_entries; i++) { ret = part_drv->get_info(desc, i, &pinfo);
if (ret) {/* no more entries in table */break;}
if (ret)continue; ptr = &part_list[str_len]; tmp_len = strlen((const char *)pinfo.name);@@ -711,9 +712,10 @@ static int gpt_enumerate(struct blk_desc *desc) /* One byte for space(" ") delimiter */ ptr[tmp_len] = ' '; }
if (*part_list)part_list[strlen(part_list) - 1] = 0;break; }
if (*part_list)part_list[strlen(part_list) - 1] = 0; debug("setenv gpt_partition_list %s\n", part_list); return env_set("gpt_partition_list", part_list);-- 2.40.1
In commands like 'ls mmc 0:f' the partition number is hexadecimal.
In command 'gpt setenv' variable gpt_partition_entry needs to be set to a hexadecimal value to allow its use as a parameter in a subsequent command.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- cmd/gpt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/gpt.c b/cmd/gpt.c index 99ca0a6163..964056bd28 100644 --- a/cmd/gpt.c +++ b/cmd/gpt.c @@ -744,7 +744,7 @@ static int gpt_setenv_part_variables(struct disk_partition *pinfo, int i) if (ret) goto fail;
- ret = env_set_ulong("gpt_partition_entry", i); + ret = env_set_hex("gpt_partition_entry", i); if (ret) goto fail;
On Fri, 25 Aug 2023 at 19:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
In commands like 'ls mmc 0:f' the partition number is hexadecimal.
In command 'gpt setenv' variable gpt_partition_entry needs to be set to a hexadecimal value to allow its use as a parameter in a subsequent command.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
cmd/gpt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
eek that is an unfortunate bug
diff --git a/cmd/gpt.c b/cmd/gpt.c index 99ca0a6163..964056bd28 100644 --- a/cmd/gpt.c +++ b/cmd/gpt.c @@ -744,7 +744,7 @@ static int gpt_setenv_part_variables(struct disk_partition *pinfo, int i) if (ret) goto fail;
ret = env_set_ulong("gpt_partition_entry", i);
ret = env_set_hex("gpt_partition_entry", i); if (ret) goto fail;-- 2.40.1
participants (2)
-
Heinrich Schuchardt -
Simon Glass