[U-Boot] [PATCH v14 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Simon Glass]

Hi Heinrich,

On Fri, 23 Nov 2018 at 00:53, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/22/18 9:50 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 19 Nov 2018 at 12:41, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 11/15/18 12:11 AM, Simon Glass wrote:
> >>> This function can be used from do_bootefi_exec() so that we use mostly the
> >>> same code for a normal EFI application and an EFI test.
> >>>
> >>> Rename the function and use it in both places.
> >>>
> >>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>> ---
> >>>
> >>> Changes in v14:
> >>> - Go back to the horrible long variable names
> >>> - Hopefully correct error paths in do_bootefi_exec()
> >>>
> >>> Changes in v13:
> >>> - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
> >>> - Drop patches previously applied
> >>>
> >>> Changes in v12: None
> >>> Changes in v11:
> >>> - Drop patches previously applied
> >>>
> >>> Changes in v9: None
> >>> Changes in v7:
> >>> - Drop patch "efi: Init the 'rows' and 'cols' variables"
> >>> - Drop patches previous applied
> >>>
> >>> Changes in v5:
> >>> - Rebase to master
> >>>
> >>> Changes in v4:
> >>> - Rebase to master
> >>>
> >>> Changes in v3:
> >>> - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
> >>>
> >>>  cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
> >>>  1 file changed, 24 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index ab7ada9f2d6..a627f689f95 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -350,6 +350,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
> >>>       return 0;
> >>>  }
> >>>
> >>> +/**
> >>> + * bootefi_run_finish() - finish up after running an EFI test
> >>> + *
> >>> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> >>> + * @image_objj: Pointer to a struct which holds the loaded image object
> >>> + */
> >>> +static void bootefi_run_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);
> >>> +}
> >>> +
> >>>  /**
> >>>   * do_bootefi_exec() - execute EFI binary
> >>>   *
> >>> @@ -390,11 +404,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> >>>                */
> >>>               ret = efi_create_handle(&mem_handle);
> >>>               if (ret != EFI_SUCCESS)
> >>> -                     goto exit;
> >>
> >> You are leaking a device path.
> >
> > I cannot see anywhere where these are freed. Are you sure this is supported?
>
> According to the UEFI standard handles are deleted when the last
> protocol is removed. See efi_uninstall_protocol_interface(). I am aware
> that our cleanup after running binaries is still incomplete.
>
> In this function you could use efi_delete_handle() in the error handler.

Actually the code is pretty impenetrable.

efi_delete_handle() takes an efi_handle_t which, confusingly, is a
pointer to an efi_object. Also we should not typedef structs in
U-Boot, particularly not with a different name.

In this case efi_create_handle() failed so we don't need to delete
that. We need to delete the efi_dp_from_mem() object. But the type of
that is struct efi_device_path, so I cannot call efi_delete_handle()
with it.

My suggestion is that if these patches land, you take a look at it and
try to figure out how to finish improving the error handling in this
function (which I started on patch v14 of this series). If I had my
way this code would be much, much simpler to read.

Regards,
Simon