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

[Simon Glass]

Hi Heinrich,

On Fri, 23 Nov 2018 at 16:23, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/22/18 9:46 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 v15:
> > - Add check for return values to bootefi_test_prepare()
> > - Drop call to efi_save_gd() in bootefi_test_prepare()
> > - Fix minor checkpatch nit with bracket
> >
> > Changes in v14:
> > - Go back to the horrible long variable names
> >
> > Changes in v13:
> > - Rebase to efi/efi-next
> >
> > Changes in v12:
> > - Rename image to image_prot
> >
> > 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 | 81 +++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 65 insertions(+), 16 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 3e37805ea13..3d98bffa542 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -453,6 +453,67 @@ 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_objp: loaded_image_infop: Pointer to a struct which will hold the
> > + *    loaded image object. This struct will be inited by this function before
> > + *    use.
> > + * @loaded_image_infop: Pointer to a struct which will hold the loaded image
> > + *    info. 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_obj **image_objp,
> > +             struct efi_loaded_image **loaded_image_infop,
> > +             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);
> > +     if (!bootefi_device_path)
> > +             return EFI_OUT_OF_RESOURCES;
> > +     bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> > +     if (!bootefi_image_path)
>
> Please, do not leak memory.

If you mean bootefi_device_path() I am not sure how to free it as per
my previous message. Also, this leak is pre-existing. I suggest you
take a look when these patches land as I think you understand the
naming in EFI better than I do. I cannot find any function that frees
a struct efi_device_path *.

>
>                 efi_free_pool(bootefi_device_path);
>
>
> > +             return EFI_OUT_OF_RESOURCES;
> > +     r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> > +                                image_objp, loaded_image_infop);
> > +     if (r)
> > +             return r;
> > +     /* efi_save_gd() has been called in efi_init_obj_list() */
>
> This comment is only of historic interest.

OK I can remove it and send v16, but I'll hold off to see what else is
happening.

>
> > +
> > +     /* Transfer environment variable efi_selftest as load options */
> > +     set_load_options(*loaded_image_infop, "efi_selftest");
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * bootefi_test_finish() - finish up after running an EFI test
> > + *
> > + * @image_obj: Pointer to a struct which holds the loaded image object
> > + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> > + */
> > +static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
> > +                             struct efi_loaded_image *loaded_image_info)
> > +{
> > +     efi_restore_gd();
> > +     free(loaded_image_info->load_options);
> > +     efi_delete_handle(&image_obj->header);
> > +}
> > +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> > +
> >  static int do_bootefi_bootmgr_exec(void)
> >  {
> >       struct efi_device_path *device_path, *file_path;
> > @@ -528,26 +589,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >               struct efi_loaded_image_obj *image_obj;
>
> In later patches we should get rid of any reference to struct
> efi_loaed_image_obj in bootefi.c. A handle is enough.

I don't understand this naming at all. Is a handle like a void *?

Regards,
Simon