Hi Heinrich,
On Fri, 23 Nov 2018 at 16:23, Heinrich Schuchardt <xypron.g...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
