[U-Boot] [PATCH v8 07/30] efi: Split out test init/uninit into functions

[Simon Glass]

Hi Heinrich,

On 20 June 2018 at 00:26, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>> We plan to run more than one EFI test. In order to avoid duplicating code,
>> create functions which handle preparing for running the test and cleaning
>> up afterwards.
>>
>> Also shorten the awfully long variable names here.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5:
>> - Drop call to efi_init_obj_list() which is now done in do_bootefi()
>>
>> Changes in v4: None
>> Changes in v3:
>> - Add new patch to split out test init/uninit into functions
>>
>> Changes in v2: None
>>
>>  cmd/bootefi.c | 87 +++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 63 insertions(+), 24 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index f55a40dc84..2dfd297e78 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -356,6 +356,60 @@ exit:
>>       return ret;
>>  }
>>
>> +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>> +/**
>> + * bootefi_test_prepare() - prepare to run an EFI test
>> + *
>> + * This sets things up so we can call EFI functions. This involves preparing
>> + * the 'gd' pointer and setting up the load ed image data structures.
>> + *
>> + * @image: Pointer to a struct which will hold the loaded image info
>> + * @obj: Pointer to a struct which will hold the loaded image object
>> + * @path: File path to the test being run (often just the test name with a
>> + *    backslash before it
>> + * @test_func: Address of the test function that is being run
>> + * @return 0 if OK, -ve on error
>> + */
>> +static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image,
>> +                                      struct efi_object *obj,
>> +                                      const char *path, ulong test_func)
>> +{
>> +     memset(image, '\0', sizeof(*image));
>> +     memset(obj, '\0', sizeof(*obj));
>
> It is unclear why you are adding the memsets here.

It's because they are passed in as pointers by callers, and otherwise
would not inited.

I will add a comment.

>
>> +     /* Construct a dummy device path */
>> +     bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>> +                                           (uintptr_t)test_func,
>> +                                           (uintptr_t)test_func);
>
> efi_dp_from_mem() may return NULL in case of an error.
>
>> +     bootefi_image_path = efi_dp_from_file(NULL, 0, path);
>> +     efi_setup_loaded_image(image, obj, bootefi_device_path,
>> +                            bootefi_image_path);
>
> The return code of efi_setup_loaded_image() should be checked. This
> function should not return 0 if the return code is not EFI_SUCCESS.

I copied the existing code, which does not check the return value
anywhere. Has this function changed, or is the current code wrong?

Regards,
Simon