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

["Vincent Stehlé" <vincent.stehle@arm.com>]

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@arm.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas@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:
>