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

[Simon Glass <sjg@chromium.org>]

Hi Alex,

On Sun, 25 Nov 2018 at 06:27, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 25.11.18 13:53, Simon Glass wrote:
> > Hi Alex,
> >
> > On Sat, 24 Nov 2018 at 14:26, Alexander Graf <agraf@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 <sjg@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