[U-Boot,v12,4/6] efi: Split out test init/uninit into functions

Message ID 20181106225744.139945-5-sjg@chromium.org
State New
Delegated to: Alexander Graf
Headers show
Series
  • efi_loader: Code refactoring and improvement
Related show

Commit Message

Simon Glass Nov. 6, 2018, 10:57 p.m.
The functions in bootefi are very long because they mix high-level code
and control with the low-level implementation. To help with this, create
functions which handle preparing for running the test and cleaning up
afterwards.

Also shorten the awfully long variable names here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v12:
- Rename image to image_prot

Changes in v11: None
Changes in v9:
- Add comments to bootefi_test_prepare() about the memset()s

Changes in v7: None
Changes in v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()

Changes in v4: None
Changes in v3:
- Add new patch to split out test init/uninit into functions

 cmd/bootefi.c | 85 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 20 deletions(-)

Comments

Alexander Graf Nov. 13, 2018, 8:16 p.m. | #1
On 06.11.18 23:57, Simon Glass wrote:
> The functions in bootefi are very long because they mix high-level code
> and control with the low-level implementation. To help with this, create
> functions which handle preparing for running the test and cleaning up
> afterwards.
> 
> Also shorten the awfully long variable names here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

This touches selftest, so I'd like to see at least an ack from Heinrich.

I personally would just extract all of the if (selftest) part into a
function and then run your later cleanup over it. The end result should
still be quite readable.


Alex

> ---
> 
> Changes in v12:
> - Rename image to image_prot
> 
> Changes in v11: None
> Changes in v9:
> - Add comments to bootefi_test_prepare() about the memset()s
> 
> Changes in v7: None
> Changes in v5:
> - Drop call to efi_init_obj_list() which is now done in do_bootefi()
> 
> Changes in v4: None
> Changes in v3:
> - Add new patch to split out test init/uninit into functions
> 
>  cmd/bootefi.c | 85 +++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 4d68d807480..0dd18d594d5 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -455,6 +455,64 @@ exit:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> +/**
> + * bootefi_test_prepare() - prepare to run an EFI test
> + *
> + * This sets things up so we can call EFI functions. This involves preparing
> + * the 'gd' pointer and setting up the load ed image data structures.
> + *
> + * @image: Pointer to a struct which will hold the loaded image info.
> + *    This struct will be inited by this function before use.
> + * @obj: Pointer to a struct which will hold the loaded image object
> + *    This struct will be inited by this function before use.
> + * @path: File path to the test being run (often just the test name with a
> + *    backslash before it
> + * @test_func: Address of the test function that is being run
> + * @return 0 if OK, -ve on error
> + */
> +static efi_status_t bootefi_test_prepare(struct efi_loaded_image **imagep,
> +					 struct efi_loaded_image_obj **objp,
> +					 const char *path, ulong test_func)
> +{
> +	efi_status_t r;
> +
> +	/* Construct a dummy device path */
> +	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					      (uintptr_t)test_func,
> +					      (uintptr_t)test_func);
> +	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> +	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> +				   objp, imagep);
> +	if (r)
> +		return r;
> +	/*
> +	 * gd lives in a fixed register which may get clobbered while we execute
> +	 * the payload. So save it here and restore it on every callback entry
> +	 */
> +	efi_save_gd();
> +
> +	/* Transfer environment variable efi_selftest as load options */
> +	set_load_options(*imagep, "efi_selftest");
> +
> +	return 0;
> +}
> +
> +/**
> + * bootefi_test_finish() - finish up after running an EFI test
> + *
> + * @image: Pointer to a struct which holds the loaded image info
> + * @obj: Pointer to a struct which holds the loaded image object
> + */
> +static void bootefi_test_finish(struct efi_loaded_image *image,
> +				struct efi_loaded_image_obj *obj)
> +{
> +	efi_restore_gd();
> +	free(image->load_options);
> +	efi_delete_handle(&obj->parent);
> +}
> +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> +
>  static int do_bootefi_bootmgr_exec(void)
>  {
>  	struct efi_device_path *device_path, *file_path;
> @@ -527,29 +585,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  #endif
>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>  	if (!strcmp(argv[1], "selftest")) {
> -		struct efi_loaded_image_obj *image_handle;
> -		struct efi_loaded_image *loaded_image_info;
> -
> -		/* Construct a dummy device path. */
> -		bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> -						      (uintptr_t)&efi_selftest,
> -						      (uintptr_t)&efi_selftest);
> -		bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest");
> -
> -		r = efi_setup_loaded_image(bootefi_device_path,
> -					   bootefi_image_path, &image_handle,
> -					   &loaded_image_info);
> -		if (r != EFI_SUCCESS)
> +		struct efi_loaded_image_obj *obj;
> +		struct efi_loaded_image *image_prot;
> +
> +		if (bootefi_test_prepare(&image_prot, &obj, "\\selftest",
> +					 (uintptr_t)&efi_selftest))
>  			return CMD_RET_FAILURE;
>  
> -		efi_save_gd();
> -		/* Transfer environment variable efi_selftest as load options */
> -		set_load_options(loaded_image_info, "efi_selftest");
>  		/* Execute the test */
> -		r = efi_selftest(image_handle, &systab);
> -		efi_restore_gd();
> -		free(loaded_image_info->load_options);
> -		efi_delete_handle(&image_handle->parent);
> +		r = efi_selftest(obj, &systab);
> +		bootefi_test_finish(image_prot, obj);
>  		return r != EFI_SUCCESS;
>  	} else
>  #endif
>

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 4d68d807480..0dd18d594d5 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -455,6 +455,64 @@  exit:
 	return ret;
 }
 
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+/**
+ * bootefi_test_prepare() - prepare to run an EFI test
+ *
+ * This sets things up so we can call EFI functions. This involves preparing
+ * the 'gd' pointer and setting up the load ed image data structures.
+ *
+ * @image: Pointer to a struct which will hold the loaded image info.
+ *    This struct will be inited by this function before use.
+ * @obj: Pointer to a struct which will hold the loaded image object
+ *    This struct will be inited by this function before use.
+ * @path: File path to the test being run (often just the test name with a
+ *    backslash before it
+ * @test_func: Address of the test function that is being run
+ * @return 0 if OK, -ve on error
+ */
+static efi_status_t bootefi_test_prepare(struct efi_loaded_image **imagep,
+					 struct efi_loaded_image_obj **objp,
+					 const char *path, ulong test_func)
+{
+	efi_status_t r;
+
+	/* Construct a dummy device path */
+	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+					      (uintptr_t)test_func,
+					      (uintptr_t)test_func);
+	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
+	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
+				   objp, imagep);
+	if (r)
+		return r;
+	/*
+	 * gd lives in a fixed register which may get clobbered while we execute
+	 * the payload. So save it here and restore it on every callback entry
+	 */
+	efi_save_gd();
+
+	/* Transfer environment variable efi_selftest as load options */
+	set_load_options(*imagep, "efi_selftest");
+
+	return 0;
+}
+
+/**
+ * bootefi_test_finish() - finish up after running an EFI test
+ *
+ * @image: Pointer to a struct which holds the loaded image info
+ * @obj: Pointer to a struct which holds the loaded image object
+ */
+static void bootefi_test_finish(struct efi_loaded_image *image,
+				struct efi_loaded_image_obj *obj)
+{
+	efi_restore_gd();
+	free(image->load_options);
+	efi_delete_handle(&obj->parent);
+}
+#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
+
 static int do_bootefi_bootmgr_exec(void)
 {
 	struct efi_device_path *device_path, *file_path;
@@ -527,29 +585,16 @@  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 #endif
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 	if (!strcmp(argv[1], "selftest")) {
-		struct efi_loaded_image_obj *image_handle;
-		struct efi_loaded_image *loaded_image_info;
-
-		/* Construct a dummy device path. */
-		bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-						      (uintptr_t)&efi_selftest,
-						      (uintptr_t)&efi_selftest);
-		bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest");
-
-		r = efi_setup_loaded_image(bootefi_device_path,
-					   bootefi_image_path, &image_handle,
-					   &loaded_image_info);
-		if (r != EFI_SUCCESS)
+		struct efi_loaded_image_obj *obj;
+		struct efi_loaded_image *image_prot;
+
+		if (bootefi_test_prepare(&image_prot, &obj, "\\selftest",
+					 (uintptr_t)&efi_selftest))
 			return CMD_RET_FAILURE;
 
-		efi_save_gd();
-		/* Transfer environment variable efi_selftest as load options */
-		set_load_options(loaded_image_info, "efi_selftest");
 		/* Execute the test */
-		r = efi_selftest(image_handle, &systab);
-		efi_restore_gd();
-		free(loaded_image_info->load_options);
-		efi_delete_handle(&image_handle->parent);
+		r = efi_selftest(obj, &systab);
+		bootefi_test_finish(image_prot, obj);
 		return r != EFI_SUCCESS;
 	} else
 #endif