Hi Alex, On Sun, 25 Nov 2018 at 06:27, Alexander Graf <ag...@suse.de> wrote: > > > > On 25.11.18 13:53, Simon Glass wrote: > > Hi Alex, > > > > On Sat, 24 Nov 2018 at 14:26, Alexander Graf <ag...@suse.de> wrote: > >> > >> > >> > >> On 22.11.18 21:46, 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 <s...@chromium.org> > >>> --- > >>> > >>> Changes in v15: > >>> - Add a comment about a leaked device path > >>> > >>> 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 0ca84ff7168..5831c991a8e 100644 > >>> --- a/cmd/bootefi.c > >>> +++ b/cmd/bootefi.c > >>> @@ -346,6 +346,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 > >>> * > >>> @@ -386,11 +400,11 @@ static efi_status_t do_bootefi_exec(void *efi, > >>> */ > >>> ret = efi_create_handle(&mem_handle); > >>> if (ret != EFI_SUCCESS) > >>> - goto exit; > >>> + return ret; /* TODO: leaks device_path */ > >>> ret = efi_add_protocol(mem_handle, &efi_guid_device_path, > >>> device_path); > >>> if (ret != EFI_SUCCESS) > >>> - goto exit; > >>> + goto err_add_protocol; > >>> } else { > >>> assert(device_path && image_path); > >>> } > >>> @@ -398,13 +412,13 @@ static efi_status_t do_bootefi_exec(void *efi, > >>> ret = bootefi_run_prepare("bootargs", device_path, image_path, > >>> &image_obj, &loaded_image_info); > >>> if (ret) > >>> - return ret; > >>> + goto err_prepare; > >>> > >>> /* Load the EFI payload */ > >>> entry = efi_load_pe(image_obj, efi, loaded_image_info); > >>> if (!entry) { > >>> ret = EFI_LOAD_ERROR; > >>> - goto exit; > >>> + goto err_prepare; > >>> } > >>> > >>> if (memdp) { > >>> @@ -424,7 +438,7 @@ static efi_status_t do_bootefi_exec(void *efi, > >>> > >>> if (setjmp(&image_obj->exit_jmp)) { > >>> ret = image_obj->exit_status; > >>> - goto exit; > >>> + goto err_prepare; > >>> } > >>> > >>> #ifdef CONFIG_ARM64 > >>> @@ -462,10 +476,11 @@ static efi_status_t do_bootefi_exec(void *efi, > >>> > >>> ret = efi_do_enter(&image_obj->header, &systab, entry); > >>> > >>> -exit: > >>> +err_prepare: > >>> /* image has returned, loaded-image obj goes *poof*: */ > >>> - if (image_obj) > >>> - efi_delete_handle(&image_obj->header); > >>> + bootefi_run_finish(image_obj, loaded_image_info); > >> > >> So here we now free loaded_image_info->load_options which we didn't do > >> before. That means the patch does change behavior. > >> > >> I think the change is correct though. For the sake of bisectability, I'd > >> prefer if you could add a tiny patch before this patch that just adds > >> the free(loaded_image_info->load_options) in this spot. We can then have > >> this refactoring really be neutral as to behavior. > > > > I agree, it worries me that we are fixing up complex code in a refactor. > > > > Can you just use v14? > > v14 has the same problem, so unfortunately not :).
Ah yes. Oh dear. I will have another go. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot