| Message ID | 20221006183351.1163531-1-festevam@denx.de |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Revert "diskpart: support non-standard GPT partition types" | expand |
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. On Thu, Oct 6, 2022 at 2:34 PM Fabio Estevam <festevam@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 <festevam@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 > > -- > 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+unsubscribe@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/20221006183351.1163531-1-festevam%40denx.de > . >
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 <festevam@denx.de > <mailto:festevam@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 <festevam@denx.de > <mailto:festevam@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 > > -- > 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+unsubscribe@googlegroups.com > <mailto:swupdate%2Bunsubscribe@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>. >
Stefano, On Thu, Oct 6, 2022 at 3:45 PM Stefano Babic <sbabic@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
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
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)); }
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 <festevam@denx.de> --- handlers/diskpart_handler.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)