| Message ID | 20221213213910.513801-3-vincent.stehle@arm.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: small hii conformance fix | expand |
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. We should test both cases separately. 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:
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: >
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@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 */ 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: >>
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) { + efi_st_error("get_package_list_handle returned %u not invalid\n", + (unsigned int)ret); + goto out; + } + result = EFI_ST_SUCCESS; out:
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(+)