diff mbox series

[v1,1/2] test: fix print_guid()

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

Commit Message

Jerome Forissier April 15, 2025, 12:53 p.m. UTC
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(+)

Comments

Heinrich Schuchardt April 15, 2025, 1:25 p.m. UTC | #1
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);
>   	}
Jerome Forissier April 15, 2025, 1:51 p.m. UTC | #2
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,
Heinrich Schuchardt April 15, 2025, 2:26 p.m. UTC | #3
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 mbox series

Patch

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);
 	}