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.ste...@arm.com>
> > Cc: Heinrich Schuchardt <xypron.g...@gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodi...@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 */
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.
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:
>
