| Message ID | 20250416074839.1267396-2-jerome.forissier@linaro.org |
|---|---|
| State | Accepted |
| Commit | d54e1004b8b171d18ad216a817018bb7aea6ec60 |
| Headers | show |
| Series | ut: fix print_guid() and enable UNIT_TEST for qemu_arm64 | expand |
Am 16. April 2025 09:48:20 MESZ schrieb Jerome Forissier <jerome.forissier@linaro.org>: >The name defined for PARTITION_SYSTEM_GUID in list_guid[] depends on >configuration options. It is "system" if CONFIG_PARTITION_TYPE_GUID is >enabled or "System Partition" if CONFIG_CMD_EFIDEBUG or CONFIG_EFI are >enabled. In addition, the unit test in test/common/print.c is incorrect >because it expects only "system" (or a hex GUID). > >Make things more consistent by using a clear and unique name: "EFI >System Partition" whatever the configuration, and update the unit test >accordingly. > >Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> LGTM Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >--- > >Changes in v3: >- Use a single entry in list_guid for PARTITION_SYSTEM_GUID > >Changes in v2: >- Remove useless braces in if expression >- Change partition name and make it the same for all configs. Update the > commit subject. > > lib/uuid.c | 9 ++++----- > test/common/print.c | 8 ++++---- > 2 files changed, 8 insertions(+), 9 deletions(-) > >diff --git a/lib/uuid.c b/lib/uuid.c >index 75658778044..6abbcf27b1f 100644 >--- a/lib/uuid.c >+++ b/lib/uuid.c >@@ -67,8 +67,11 @@ static const struct { > efi_guid_t guid; > } list_guid[] = { > #ifndef USE_HOSTCC >+#if defined(CONFIG_PARTITION_TYPE_GUID) || defined(CONFIG_CMD_EFIDEBUG) || \ >+ defined(CONFIG_EFI) >+ {"EFI System Partition", PARTITION_SYSTEM_GUID}, >+#endif > #ifdef CONFIG_PARTITION_TYPE_GUID >- {"system", PARTITION_SYSTEM_GUID}, > {"mbr", LEGACY_MBR_PARTITION_GUID}, > {"msft", PARTITION_MSFT_RESERVED_GUID}, > {"data", PARTITION_BASIC_DATA_GUID}, >@@ -182,10 +185,6 @@ static const struct { > { > "TCG2", > EFI_TCG2_PROTOCOL_GUID, >- }, >- { >- "System Partition", >- PARTITION_SYSTEM_GUID > }, > { > "Firmware Management", >diff --git a/test/common/print.c b/test/common/print.c >index e3711b10809..c48efc2783f 100644 >--- a/test/common/print.c >+++ b/test/common/print.c >@@ -45,11 +45,11 @@ static int print_guid(struct unit_test_state *uts) > sprintf(str, "%pUL", guid); > ut_asserteq_str("04030201-0605-0807-090A-0B0C0D0E0F10", str); > sprintf(str, "%pUs", guid_esp); >- if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */ >- ut_asserteq_str("system", str); >- } else { >+ if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) || >+ IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI)) >+ ut_asserteq_str("EFI System Partition", str); >+ else > ut_asserteq_str("c12a7328-f81f-11d2-ba4b-00a0c93ec93b", str); >- } > ret = snprintf(str, 4, "%pUL", guid); > ut_asserteq(0, str[3]); > ut_asserteq(36, ret);
On Wed, 16 Apr 2025 at 10:48, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > The name defined for PARTITION_SYSTEM_GUID in list_guid[] depends on > configuration options. It is "system" if CONFIG_PARTITION_TYPE_GUID is > enabled or "System Partition" if CONFIG_CMD_EFIDEBUG or CONFIG_EFI are > enabled. In addition, the unit test in test/common/print.c is incorrect > because it expects only "system" (or a hex GUID). > > Make things more consistent by using a clear and unique name: "EFI > System Partition" whatever the configuration, and update the unit test > accordingly. > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > > Changes in v3: > - Use a single entry in list_guid for PARTITION_SYSTEM_GUID > > Changes in v2: > - Remove useless braces in if expression > - Change partition name and make it the same for all configs. Update the > commit subject. > > lib/uuid.c | 9 ++++----- > test/common/print.c | 8 ++++---- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/lib/uuid.c b/lib/uuid.c > index 75658778044..6abbcf27b1f 100644 > --- a/lib/uuid.c > +++ b/lib/uuid.c > @@ -67,8 +67,11 @@ static const struct { > efi_guid_t guid; > } list_guid[] = { > #ifndef USE_HOSTCC > +#if defined(CONFIG_PARTITION_TYPE_GUID) || defined(CONFIG_CMD_EFIDEBUG) || \ > + defined(CONFIG_EFI) > + {"EFI System Partition", PARTITION_SYSTEM_GUID}, > +#endif > #ifdef CONFIG_PARTITION_TYPE_GUID > - {"system", PARTITION_SYSTEM_GUID}, > {"mbr", LEGACY_MBR_PARTITION_GUID}, > {"msft", PARTITION_MSFT_RESERVED_GUID}, > {"data", PARTITION_BASIC_DATA_GUID}, > @@ -182,10 +185,6 @@ static const struct { > { > "TCG2", > EFI_TCG2_PROTOCOL_GUID, > - }, > - { > - "System Partition", > - PARTITION_SYSTEM_GUID > }, > { > "Firmware Management", > diff --git a/test/common/print.c b/test/common/print.c > index e3711b10809..c48efc2783f 100644 > --- a/test/common/print.c > +++ b/test/common/print.c > @@ -45,11 +45,11 @@ static int print_guid(struct unit_test_state *uts) > sprintf(str, "%pUL", guid); > ut_asserteq_str("04030201-0605-0807-090A-0B0C0D0E0F10", str); > sprintf(str, "%pUs", guid_esp); > - if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */ > - ut_asserteq_str("system", str); > - } else { > + if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) || > + IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI)) > + ut_asserteq_str("EFI System Partition", str); > + else > ut_asserteq_str("c12a7328-f81f-11d2-ba4b-00a0c93ec93b", str); > - } > ret = snprintf(str, 4, "%pUL", guid); > ut_asserteq(0, str[3]); > ut_asserteq(36, ret); > -- > 2.43.0 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Hi Jerome,
Sorry for my late review of your patch,
but I see a side effect exist for this patch
the modified string in list_guid[] is used in uuid_guid_get_bin()
called by uuid_str_to_bin() with CONFIG_PARTITION_TYPE_GUID activated
to support shortname for knwon GUID partition in gpt command with
type=<UUID>
see doc/README.gpt
I add this behavior in
------------------------------------------------------------------------------------------------------------------------------
commit bcb41dcaefacdd4cf0f679b34224cb3d997ee8b4 Author: Patrick Delaunay
<patrick.delaunay73@gmail.com> Tue Oct 27 11:00:28 2015 Committer: Tom
Rini <trini@konsulko.com> Thu Nov 12 21:58:58 2015 uuid: add selection
by string for known partition type GUID short strings can be used in
type parameter of gpt command to replace the guid string for the types
known by u-boot partitions = name=boot,size=0x6bc00,type=data; \
name=root,size=0x7538ba00,type=linux; gpt write mmc 0 $partitions and
they are also used to display the type of partition in "part list"
command Partition Map for MMC device 0 -- Partition Type: EFI Part Start
LBA End LBA Name Attributes Type GUID Partition GUID 1 0x00000022
0x0000037f "boot" attrs: 0x0000000000000000 type:
ebd0a0a2-b9e5-4433-87c0-68b6b72699c7 type: data guid:
d117f98e-6f2c-d04b-a5b2-331a19f91cb2 2 0x00000380 0x003a9fdc "root"
attrs: 0x0000000000000000 type: 0fc63daf-8483-4772-8e79-3d69d8477de4
type: linux guid: 25718777-d0ad-7443-9e60-02cb591c9737 Signed-off-by:
Patrick Delaunay <patrick.delaunay73@gmail.com>
--------------------------------------------------------------------------------------------------
I found at least 2 usage of this string "system" in master branch.
1- board/samsung/e850-96/e850-96.env:10:
partitions=name=esp,start=512K,size=128M,bootable,type=system;
partitions+=name=rootfs,size=-,bootable,type=linux
2- arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c:1151
case PART_ESP:
/* EFI System Partition */
type_str = "system"
....
offset += snprintf(buf + offset,
buflen - offset,
",type=%s", type_str);
With your patch the short name for ESP UUID change from "system" to "EFI System Partition"
so we have with space in the part UUID name and the old short name "system" for CONFIG_PARTITION_TYPE_GUID is no more supported
the SPACE in GPT partition could cause issue for gpt command if we try to use this name
cmd/gpt.c:535:
/* guid */
val = extract_val(tok, "type");
if (val) {
and extract_val() use space as token separator....
we can keep the backward compatibility for the exist shortname "system" with:
#if defined(CONFIG_PARTITION_TYPE_GUID) || defined(CONFIG_CMD_EFIDEBUG) || \
defined(CONFIG_EFI)
+ {"EFI System Partition", PARTITION_SYSTEM_GUID},
{"system", PARTITION_SYSTEM_GUID},
#endif
with this simple modification
- first occurrence is used to display information (long name = uuid_guid_get_str)
- second occurence i used for gpt command parameter 'type=system' with ( uuid_str_to_bin => uuid_guid_get_bin)
But I prefer manage short name for gpt command and long name for description (printf with the '%pUs' format)
firectly in the array (interdependently of CONFIG_PARTITION_TYPE_GUID)and change the functions uuid_guid_get_bin() / uuid_guid_get_str() to
use correctly the field .string OR .description
Moreover, as the function are uuid_str_to_bin() / uuid_guid_get_str()
are no more under CONFIG_PARTITION_TYPE_GUID, since commit 31ce367cd100 ("lib/uuid.c: change prototype of uuid_guid_get_str()")
and commit c1528f324c60 ("lib: compile uuid_guid_get_str if CONFIG_LIB_UUID=y")
I don't see any reason to continue to use this compilation flag for list_guid[]
and the array be be removed by linker if it is not used...
I propose this modification in the serie
https://patchwork.ozlabs.org/project/uboot/list/?series=457430 with the
fix in
https://patchwork.ozlabs.org/project/uboot/patch/20250519110417.1.Ie741b1ca358414a1d718dca0667ac44eefc9227b@changeid/
"[1/3] lib/uuid.c: restore support of system partition type for ESP" All
feedbacks are welcome....
PS: we can also assume to break the backward compatibility for "system" short name and use a better short name as "ESP" for "EFI system partition" if you prefer.
Regards
Patrick
On 4/16/25 09:48, Jerome Forissier wrote:
> The name defined for PARTITION_SYSTEM_GUID in list_guid[] depends on
> configuration options. It is "system" if CONFIG_PARTITION_TYPE_GUID is
> enabled or "System Partition" if CONFIG_CMD_EFIDEBUG or CONFIG_EFI are
> enabled. In addition, the unit test in test/common/print.c is incorrect
> because it expects only "system" (or a hex GUID).
>
> Make things more consistent by using a clear and unique name: "EFI
> System Partition" whatever the configuration, and update the unit test
> accordingly.
>
> Signed-off-by: Jerome Forissier<jerome.forissier@linaro.org>
> Suggested-by: Heinrich Schuchardt<xypron.glpk@gmx.de>
> ---
>
> Changes in v3:
> - Use a single entry in list_guid for PARTITION_SYSTEM_GUID
>
> Changes in v2:
> - Remove useless braces in if expression
> - Change partition name and make it the same for all configs. Update the
> commit subject.
>
> lib/uuid.c | 9 ++++-----
> test/common/print.c | 8 ++++----
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/lib/uuid.c b/lib/uuid.c
> index 75658778044..6abbcf27b1f 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -67,8 +67,11 @@ static const struct {
> efi_guid_t guid;
> } list_guid[] = {
> #ifndef USE_HOSTCC
> +#if defined(CONFIG_PARTITION_TYPE_GUID) || defined(CONFIG_CMD_EFIDEBUG) || \
> + defined(CONFIG_EFI)
> + {"EFI System Partition", PARTITION_SYSTEM_GUID},
> +#endif
> #ifdef CONFIG_PARTITION_TYPE_GUID
> - {"system", PARTITION_SYSTEM_GUID},
> {"mbr", LEGACY_MBR_PARTITION_GUID},
> {"msft", PARTITION_MSFT_RESERVED_GUID},
> {"data", PARTITION_BASIC_DATA_GUID},
> @@ -182,10 +185,6 @@ static const struct {
> {
> "TCG2",
> EFI_TCG2_PROTOCOL_GUID,
> - },
> - {
> - "System Partition",
> - PARTITION_SYSTEM_GUID
> },
> {
> "Firmware Management",
> diff --git a/test/common/print.c b/test/common/print.c
> index e3711b10809..c48efc2783f 100644
> --- a/test/common/print.c
> +++ b/test/common/print.c
> @@ -45,11 +45,11 @@ static int print_guid(struct unit_test_state *uts)
> sprintf(str, "%pUL", guid);
> ut_asserteq_str("04030201-0605-0807-090A-0B0C0D0E0F10", str);
> sprintf(str, "%pUs", guid_esp);
> - if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */
> - ut_asserteq_str("system", str);
> - } else {
> + if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) ||
> + IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI))
> + ut_asserteq_str("EFI System Partition", str);
> + else
> ut_asserteq_str("c12a7328-f81f-11d2-ba4b-00a0c93ec93b", str);
> - }
> ret = snprintf(str, 4, "%pUL", guid);
> ut_asserteq(0, str[3]);
> ut_asserteq(36, ret);
diff --git a/lib/uuid.c b/lib/uuid.c index 75658778044..6abbcf27b1f 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -67,8 +67,11 @@ static const struct { efi_guid_t guid; } list_guid[] = { #ifndef USE_HOSTCC +#if defined(CONFIG_PARTITION_TYPE_GUID) || defined(CONFIG_CMD_EFIDEBUG) || \ + defined(CONFIG_EFI) + {"EFI System Partition", PARTITION_SYSTEM_GUID}, +#endif #ifdef CONFIG_PARTITION_TYPE_GUID - {"system", PARTITION_SYSTEM_GUID}, {"mbr", LEGACY_MBR_PARTITION_GUID}, {"msft", PARTITION_MSFT_RESERVED_GUID}, {"data", PARTITION_BASIC_DATA_GUID}, @@ -182,10 +185,6 @@ static const struct { { "TCG2", EFI_TCG2_PROTOCOL_GUID, - }, - { - "System Partition", - PARTITION_SYSTEM_GUID }, { "Firmware Management", diff --git a/test/common/print.c b/test/common/print.c index e3711b10809..c48efc2783f 100644 --- a/test/common/print.c +++ b/test/common/print.c @@ -45,11 +45,11 @@ static int print_guid(struct unit_test_state *uts) sprintf(str, "%pUL", guid); ut_asserteq_str("04030201-0605-0807-090A-0B0C0D0E0F10", str); sprintf(str, "%pUs", guid_esp); - if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */ - ut_asserteq_str("system", str); - } else { + if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) || + IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI)) + ut_asserteq_str("EFI System Partition", str); + else ut_asserteq_str("c12a7328-f81f-11d2-ba4b-00a0c93ec93b", str); - } ret = snprintf(str, 4, "%pUL", guid); ut_asserteq(0, str[3]); ut_asserteq(36, ret);
The name defined for PARTITION_SYSTEM_GUID in list_guid[] depends on configuration options. It is "system" if CONFIG_PARTITION_TYPE_GUID is enabled or "System Partition" if CONFIG_CMD_EFIDEBUG or CONFIG_EFI are enabled. In addition, the unit test in test/common/print.c is incorrect because it expects only "system" (or a hex GUID). Make things more consistent by using a clear and unique name: "EFI System Partition" whatever the configuration, and update the unit test accordingly. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- Changes in v3: - Use a single entry in list_guid for PARTITION_SYSTEM_GUID Changes in v2: - Remove useless braces in if expression - Change partition name and make it the same for all configs. Update the commit subject. lib/uuid.c | 9 ++++----- test/common/print.c | 8 ++++---- 2 files changed, 8 insertions(+), 9 deletions(-)