[PATCH 2/2] efi_selftest: add hii database protocol test case

[Heinrich Schuchardt]

On 1/5/23 15:48, Vincent Stehlé wrote:
> On Thu, Dec 22, 2022 at 10:45:22AM +0100, Heinrich Schuchardt wrote:
>
> Hi Heinrich,
>
> Happy new year, and thank you for the reviews.
> More comments below.
>
>> On 12/13/22 22:39, Vincent Stehlé wrote:
>>> Add a test for the case when the HII database protocol
>>> get_package_list_handle() function is called with an invalid package list
>>> handle.
>>>
>>> Signed-off-by: Vincent Stehlé <vincent.stehle at arm.com>
>>> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>> ---
>>>    lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c
>>> index eaf3b0995d4..8a038d9f534 100644
>>> --- a/lib/efi_selftest/efi_selftest_hii.c
>>> +++ b/lib/efi_selftest/efi_selftest_hii.c
>>> @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void)
>>>    		goto out;
>>>    	}
>>>
>>> +	/* Invalid package list handle. */
>>> +	driver_handle = NULL;
>>> +	ret = hii_database_protocol->get_package_list_handle(
>>> +			hii_database_protocol, NULL, &driver_handle);
>>> +	if (ret != EFI_INVALID_PARAMETER) {
>>
>> Here it is unclear, if you get EFI_INVALID_PARAMETER because the
>> PackageListHandle is invalid or DriverHandle is NULL.
>
> In principle, the value used to initialize the (output) parameter driver_handle
> should not affect the test. Also, we are initializing driver_handle in exactly
> the same way as was done in the previous test, which we know succeeded.
>
> Anyway, if you think it makes the test more readable I can change the
> initialization to something like the following:
>
>    driver_handle = (efi_handle_t)0x23456789; /* dummy */
>
>>
>> We should test both cases separately.
>
> Would you prefer something like the following couple of tests?
>
>    /* Invalid package list handle. */
>    driver_handle = (efi_handle_t)0x23456789; /* dummy */

Let's use an address that never will be allocated.
driver_handle = (efi_handle_t)~1UL;

>    ret = hii_database_protocol->get_package_list_handle(
>                    hii_database_protocol, NULL, &driver_handle);
>
>    /* Invalid driver handle. */
>    ret = hii_database_protocol->get_package_list_handle(
>                    hii_database_protocol, handle, NULL);
>
> I could verify that EFI_INVALID_PARAMETER is indeed returned in both cases.
> Just let me know and I will post a v2.

Looks like the right path forward.

Best regards

Heinrich

>
> Best regards,
> Vincent.
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> +		efi_st_error("get_package_list_handle returned %u not invalid\n",
>>> +			     (unsigned int)ret);
>>> +		goto out;
>>> +	}
>>> +
>>>    	result = EFI_ST_SUCCESS;
>>>
>>>    out:
>>