[U-Boot] [PATCH v11 4/6] efi: Split out test init/uninit into functions

[Simon Glass]

HI Heinrich,

On 15 October 2018 at 11:34, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 10/15/2018 04:17 PM, Simon Glass wrote:
>> The functions in bootefi are very long because they mix high-level code
>> and control with the low-level implementation. To help with this, 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 v11: None
>> Changes in v9:
>> - Add comments to bootefi_test_prepare() about the memset()s
>>
>> Changes in v7: 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
>>
>>  cmd/bootefi.c | 90 +++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 65 insertions(+), 25 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 82d755ceb31..88b8b0172db 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -454,6 +454,64 @@ 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.
>> + *    This struct will be inited by this function before use.
>> + * @obj: Pointer to a struct which will hold the loaded image object
>> + *    This struct will be inited by this function before use.
>> + * @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 **imagep,
>> +                                      struct efi_loaded_image_obj **objp,
>> +                                      const char *path, ulong test_func)
>> +{
>> +     efi_status_t r;
>> +
>> +     /* Construct a dummy device path */
>> +     bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>> +                                           (uintptr_t)test_func,
>> +                                           (uintptr_t)test_func);
>> +     bootefi_image_path = efi_dp_from_file(NULL, 0, path);
>> +     r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
>> +                                objp, imagep);
>> +     if (r)
>> +             return r;
>> +     /*
>> +      * gd lives in a fixed register which may get clobbered while we execute
>> +      * the payload. So save it here and restore it on every callback entry
>> +      */
>> +     efi_save_gd();
>> +
>> +     /* Transfer environment variable efi_selftest as load options */
>> +     set_load_options(*imagep, "efi_selftest");
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * bootefi_test_finish() - finish up after running an EFI test
>> + *
>> + * @image: Pointer to a struct which holds the loaded image info
>> + * @obj: Pointer to a struct which holds the loaded image object
>> + */
>> +static void bootefi_test_finish(struct efi_loaded_image *image,
>> +                             struct efi_loaded_image_obj *obj)
>> +{
>> +     efi_restore_gd();
>> +     free(image->load_options);
>> +     efi_delete_handle(&obj->parent);
>> +}
>> +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>> +
>>  static int do_bootefi_bootmgr_exec(void)
>>  {
>>       struct efi_device_path *device_path, *file_path;
>> @@ -532,34 +590,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>  #endif
>>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>       if (!strcmp(argv[1], "selftest")) {
>
> Why not move the whole body of the if clause to a single separate
> function and do the same for 'hello'.

Because the goal here is to have a 'prepare' function and a 'finish'
function, to make it easier to see what is going on (which is that we
are running a test).

>
>> -             struct efi_loaded_image_obj *image_handle;
>> -             struct efi_loaded_image *loaded_image_info;
>> -
>> -             /* Construct a dummy device path. */
>> -             bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>> -                                                   (uintptr_t)&efi_selftest,
>> -                                                   (uintptr_t)&efi_selftest);
>> -             bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest");
>> -
>> -             r = efi_setup_loaded_image(bootefi_device_path,
>> -                                        bootefi_image_path, &image_handle,
>> -                                        &loaded_image_info);
>> -             if (r != EFI_SUCCESS)
>> +             struct efi_loaded_image_obj *obj;
>
> obj what? Use a speaking name: img_handle.

This is exactly what I find confusing about EFI. What does this have
to do with a handle? It is an image object, isn't it? But then in the
next line we have an image.So I don't want 'img' in my variable since
the next variable is an image:

>
>> +             struct efi_loaded_image *image;
>
> This name obscures the variable content. The variable holds the
> EFI_LOADED_IMAGE_PROTOCOL. So you could call it img_prot.

OK I'll change this one, but it's pretty horrible. A loaded image
protocol? What does that even mean in the real world? Just
gobbledygook I think.

Regards,
Simon