[PATCH] Revert "diskpart: support non-standard GPT partition types"

[Fabio Estevam]

This reverts commit 8064baed0009c9418b298518f5ae210151455db3.

After upgrading swupdate from 2021.04 to 2022.05 in kirkstone, the eMMC
partitioning no longer works:

[ERROR] : SWUPDATE failed [0] ERROR : Partition table cannot be applied: -22

The original sw-description passed the partition type as "type=Linux":

partitions: (
{
type = "diskpart";
device = "/dev/mmcblk0";

properties: {
labeltype = "gpt";
partition-1 = ["size=2G", "start=2048", "name=root1", "type=Linux"];

Linux is not a valid GUID string, so the original behavior was to fallback
to GPT_DEFAULT_ENTRY_TYPE.

After such commit, the unsupported "Linux" GUID string is passed to
libfdisk causing fdisk_apply_table() to fail.

To avoid regressions, revert the commit so that the original behavior
can be preserved.

Thanks to Stefano for his help in debugging the failure.

Signed-off-by: Fabio Estevam <fest...@denx.de>
---
handlers/diskpart_handler.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
index 27534f6..7ba1fb0 100644
--- a/handlers/diskpart_handler.c
+++ b/handlers/diskpart_handler.c
@@ -636,13 +636,9 @@ static int diskpart_fill_table(struct fdisk_context *cxt, struct diskpart_table
* GPT uses strings instead of hex code for partition type
*/
if (fdisk_is_label(PARENT(cxt), GPT)) {
- if (part->type[0]) {
- parttype = fdisk_label_get_parttype_from_string(lb, part->type);
- if (!parttype)
- parttype = fdisk_new_unknown_parttype(0, part->type);
- } else {
+ parttype = fdisk_label_get_parttype_from_string(lb, part->type);
+ if (!parttype)
parttype = fdisk_label_get_parttype_from_string(lb, GPT_DEFAULT_ENTRY_TYPE);
- }
} else {
parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, NULL, 16));
}
--
2.25.1

[Steven Walter]

Is it really a regression that swupdate now explicitly fails when you ask it to do something incorrect?

---

From: Fabio Estevam <fest...@denx.de>
Sent: Thursday, October 6, 2022 2:33 PM
To: swup...@googlegroups.com <swup...@googlegroups.com>
Cc: sba...@denx.de <sba...@denx.de>; Steven Walter <steven...@lexmark.com>; bkyler...@gmail.com <bkyler...@gmail.com>; Fabio Estevam <fest...@denx.de>
Subject: [PATCH] Revert "diskpart: support non-standard GPT partition types"

 

[You don't often get email from fest...@denx.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

[Kyle Russell]

The behavior you're describing is not documented.  No sensible documentation would require you to explicitly list something incorrect (like "type=Linux") in order to get default behavior.  If you just want GPT_DEFAULT_ENTRY_TYPE, then remove "type=Linux" from your sw-description, or add a special translation case for "Linux" in swupdate by doing strcmp() on part->type.

Reverting this is the wrong decision because you're breaking correct behavior that allows you to use **valid** GUIDs unknown to libfdisk.

--
You received this message because you are subscribed to the Google Groups "swupdate" group.
To unsubscribe from this group and stop receiving emails from it, send an email to swupdate+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/swupdate/20221006183351.1163531-1-festevam%40denx.de.

[Stefano Babic]

Hi Kyle,

On 06.10.22 21:31, Kyle Russell wrote:
> The behavior you're describing is not documented.  No sensible
> documentation would require you to explicitly list something incorrect
> (like "type=Linux") in order to get default behavior.  If you just want
> GPT_DEFAULT_ENTRY_TYPE, then remove "type=Linux" from your
> sw-description, or add a special translation case for "Linux" in
> swupdate by doing strcmp() on part->type.
>

This is correct and this is the way I told Fabio to let update working
with 2022.05. However:

> Reverting this is the wrong decision because you're breaking correct
> behavior that allows you to use **valid** GUIDs unknown to libfdisk.
>

This is part not clear to me after reading back the old thread and the
patch itself. libfdisk runs the internal uuid_parse() function that
looks up in the table created from include/pt-gpt-partnames.h (both in
libfdisk). If no entry is found, libfdisk raises an error and partition
table cannot be applied. That means that if
fdisk_label_get_parttype_from_string() returns with failure,
fdisk_apply_table() will fail. So we already know that fails before
applying the table, and SWUpdate should raise an error or switch back to
a default GUID like we had before. The commit here will suppose (am I
wrong ?) that we can still apply the table even if the GUID wasn't found
and it is unknown.

So it is not clear the benefits of the commit here, because libfdisk
won't take the GUID on the disk, but expects to get as input a GUID from
its internal table (that is updated at each libfdisk's version, of
course) - tested with util-linux 2.38.

Best regards,
Stefano

> On Thu, Oct 6, 2022 at 2:34 PM Fabio Estevam <fest...@denx.de
> <mailto:fest...@denx.de>> wrote:
>
> This reverts commit 8064baed0009c9418b298518f5ae210151455db3.
>
> After upgrading swupdate from 2021.04 to 2022.05 in kirkstone, the eMMC
> partitioning no longer works:
>
> [ERROR] : SWUPDATE failed [0] ERROR : Partition table cannot be
> applied: -22
>
> The original sw-description passed the partition type as "type=Linux":
>
> partitions: (
> {
>         type = "diskpart";
>         device = "/dev/mmcblk0";
>
>                 properties: {
>                         labeltype = "gpt";
>                         partition-1 = ["size=2G", "start=2048",
> "name=root1", "type=Linux"];
>
> Linux is not a valid GUID string, so the original behavior was to
> fallback
> to GPT_DEFAULT_ENTRY_TYPE.
>
> After such commit, the unsupported "Linux" GUID string is passed to
> libfdisk causing fdisk_apply_table() to fail.
>
> To avoid regressions, revert the commit so that the original behavior
> can be preserved.
>
> Thanks to Stefano for his help in debugging the failure.
>
> Signed-off-by: Fabio Estevam <fest...@denx.de

> <mailto:fest...@denx.de>>

> <mailto:swupdate%2Bunsu...@googlegroups.com>.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/swupdate/20221006183351.1163531-1-festevam%40denx.de

> <https://groups.google.com/d/msgid/swupdate/20221006183351.1163531-1-festevam%40denx.de>.
>

[Kyle Russell]

Stefano,

On Thu, Oct 6, 2022 at 3:45 PM Stefano Babic <sba...@denx.de> wrote:

This is part not clear to me after reading back the old thread and the
patch itself. libfdisk runs the internal uuid_parse() function that
looks up in the table created from include/pt-gpt-partnames.h (both in
libfdisk). If no entry is found, libfdisk raises an error and partition
table cannot be applied. That means that if
fdisk_label_get_parttype_from_string() returns with failure,
fdisk_apply_table() will fail. So we already know that fails before
applying the table, and SWUpdate should raise an error or switch back to
a default GUID like we had before. The commit here will suppose (am I
wrong ?) that we can still apply the table even if the GUID wasn't found
and it is unknown.

An error from fdisk_label_get_parttype_from_string() just means that the GUID wasn't found

in libfdisk's limited table, but it does not mean that the provided GUID was invalid.

libfdisk doesn't know about all possible GUIDs, which is precisely why they provide

fdisk_new_unknown_parttype().  But if it's a valid GUID, it can still be used as a

parttype.

 

So it is not clear the benefits of the commit here, because libfdisk
won't take the GUID on the disk, but expects to get as input a GUID from
its internal table (that is updated at each libfdisk's version, of
course) - tested with util-linux 2.38.

libfdisk still takes the GUID on the disk.  The benefit is that swupdate won't

completely destroy a partition with a valid GUID that's unknown to libfdisk.

If no "type" is specified in the sw-description, swupdate should not change the

GUID in the existing partition table, even if it's not GPT_DEFAULT_ENTRY_TYPE.

Hope that helps,

Kyle

[Fabio Estevam]

Hi Kyle,

On 06/10/2022 17:03, Kyle Russell wrote:

> libfdisk still takes the GUID on the disk. The benefit is that
> swupdate won't
> completely destroy a partition with a valid GUID that's unknown to
> libfdisk.
> If no "type" is specified in the sw-description, swupdate should not
> change the
> GUID in the existing partition table, even if it's not
> GPT_DEFAULT_ENTRY_TYPE.
>
> Hope that helps,

Thanks for the explanation. Now it is clear.

We can drop my patch.

Regards,

Fabio Estevam