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.
Thanks,
Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
