Hi Heinrich, On Fri, 23 Nov 2018 at 00:53, Heinrich Schuchardt <[email protected]> wrote: > > On 11/22/18 9:50 PM, Simon Glass wrote: > > Hi Heinrich, > > > > On Mon, 19 Nov 2018 at 12:41, Heinrich Schuchardt <[email protected]> > > 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 <[email protected]> > >>> --- > >>> > >>> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
