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