| Message ID | 20250415125345.427420-2-jerome.forissier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ut: fix print_guid() and enable UNIT_TEST for qemu_arm64 | expand |
On 15.04.25 14:53, Jerome Forissier wrote: > The %pUs format specifier prints the system partition GUID as "System > Partition" if PARTITION_TYPE_GUID is disabled but either CMD_EFIDEBUG > or EFI are enabled. See list_guid[] in lib/uuid.c. Fix the print_guid() > unit test accordingly. > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > --- > > test/common/print.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/test/common/print.c b/test/common/print.c > index e3711b10809..574c84b2eee 100644 > --- a/test/common/print.c > +++ b/test/common/print.c > @@ -47,6 +47,8 @@ static int print_guid(struct unit_test_state *uts) > sprintf(str, "%pUs", guid_esp); > if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */ I would not know why a brace should be needed for a define using {( some expressions }). The problem should be resolved since fa847bb409d6 ("test: Wrap assert macros in ({ ... }) and fix missing semicolons"). > ut_asserteq_str("system", str); > + } else if (IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI)) { > + ut_asserteq_str("System Partition", str); Printing two different strings depending on configuration settings is not very helpful for users. Can we print "ESP" in all cases, please? Best regards Heinrich > } else { > ut_asserteq_str("c12a7328-f81f-11d2-ba4b-00a0c93ec93b", str); > }
Hi Heinrich, On 4/15/25 15:25, Heinrich Schuchardt wrote: > On 15.04.25 14:53, Jerome Forissier wrote: >> The %pUs format specifier prints the system partition GUID as "System >> Partition" if PARTITION_TYPE_GUID is disabled but either CMD_EFIDEBUG >> or EFI are enabled. See list_guid[] in lib/uuid.c. Fix the print_guid() >> unit test accordingly. >> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >> --- >> >> test/common/print.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/test/common/print.c b/test/common/print.c >> index e3711b10809..574c84b2eee 100644 >> --- a/test/common/print.c >> +++ b/test/common/print.c >> @@ -47,6 +47,8 @@ static int print_guid(struct unit_test_state *uts) >> sprintf(str, "%pUs", guid_esp); >> if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */ > > I would not know why a brace should be needed for a define using > {( some expressions }). > > The problem should be resolved since fa847bb409d6 ("test: Wrap assert macros in ({ ... }) and fix missing semicolons"). Most probably, yes. I'll reformat in v2. >> ut_asserteq_str("system", str); >> + } else if (IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI)) { >> + ut_asserteq_str("System Partition", str); > > Printing two different strings depending on configuration settings is not very helpful for users. Agreed. > Can we print "ESP" in all cases, please? Fine with me in principle but... - Don't we risk breaking things which would depend on the partition name? - I find "ESP" a bit cryptic. I would suggest "EFI System Partition" but it is quite long and I see that CONFIG_PARTITION_TYPE_GUID defines only short names ("system", "mbr", "msft"...) so there must be a reason. Regards,
On 15.04.25 15:51, Jerome Forissier wrote: > Hi Heinrich, > > On 4/15/25 15:25, Heinrich Schuchardt wrote: >> On 15.04.25 14:53, Jerome Forissier wrote: >>> The %pUs format specifier prints the system partition GUID as "System >>> Partition" if PARTITION_TYPE_GUID is disabled but either CMD_EFIDEBUG >>> or EFI are enabled. See list_guid[] in lib/uuid.c. Fix the print_guid() >>> unit test accordingly. >>> >>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >>> --- >>> >>> test/common/print.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/test/common/print.c b/test/common/print.c >>> index e3711b10809..574c84b2eee 100644 >>> --- a/test/common/print.c >>> +++ b/test/common/print.c >>> @@ -47,6 +47,8 @@ static int print_guid(struct unit_test_state *uts) >>> sprintf(str, "%pUs", guid_esp); >>> if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */ >> >> I would not know why a brace should be needed for a define using >> {( some expressions }). >> >> The problem should be resolved since fa847bb409d6 ("test: Wrap assert macros in ({ ... }) and fix missing semicolons"). > > Most probably, yes. I'll reformat in v2. > >>> ut_asserteq_str("system", str); >>> + } else if (IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI)) { >>> + ut_asserteq_str("System Partition", str); >> >> Printing two different strings depending on configuration settings is not very helpful for users. > > Agreed. > >> Can we print "ESP" in all cases, please? > > Fine with me in principle but... > - Don't we risk breaking things which would depend on the partition name? > - I find "ESP" a bit cryptic. I would suggest "EFI System Partition" but it Fine with me. > is quite long and I see that CONFIG_PARTITION_TYPE_GUID defines only short > names ("system", "mbr", "msft"...) so there must be a reason. The place where the string is printed is part_print_efi(). It is on a separate line: Partition Map for host device 0 -- Partition Type: EFI Part Start LBA End LBA Name Attributes Type GUID Partition GUID 1 0x00000800 0x0003f7ff "EFI system partition" attrs: 0x0000000000000000 type: c12a7328-f81f-11d2-ba4b-00a0c93ec93b (system) guid: 3795c782-a55d-490b-b0f6-c1da67bef048 The gpt command uses GUIDs and not strings. Thus any string value should be fine. Best regards Heinrich > > Regards,
diff --git a/test/common/print.c b/test/common/print.c index e3711b10809..574c84b2eee 100644 --- a/test/common/print.c +++ b/test/common/print.c @@ -47,6 +47,8 @@ static int print_guid(struct unit_test_state *uts) sprintf(str, "%pUs", guid_esp); if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID)) { /* brace needed */ ut_asserteq_str("system", str); + } else if (IS_ENABLED(CONFIG_CMD_EFIDEBUG) || IS_ENABLED(CONFIG_EFI)) { + ut_asserteq_str("System Partition", str); } else { ut_asserteq_str("c12a7328-f81f-11d2-ba4b-00a0c93ec93b", str); }
The %pUs format specifier prints the system partition GUID as "System Partition" if PARTITION_TYPE_GUID is disabled but either CMD_EFIDEBUG or EFI are enabled. See list_guid[] in lib/uuid.c. Fix the print_guid() unit test accordingly. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> --- test/common/print.c | 2 ++ 1 file changed, 2 insertions(+)