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

[Alexander Graf]

On 25.11.18 13:53, Simon Glass wrote:
> Hi Alex,
> 
> On Sat, 24 Nov 2018 at 14:26, Alexander Graf <agraf at 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 at 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 :).


Alex