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

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.

Thanks,

Alex