[PATCH] efi_loader: simplify efi_load_from_path

[PATCH] efi_loader: simplify efi_load_from_path

[Ilias Apalodimas]

The current implementation efi_load_from_path is a bit confusing.
First of all it tries to check the device path to make sure it contains
the path to the device plus the media path with the filename and nothing in
between.  But that should already be valid since U-Boot constructs those
device paths.
On top of that it tries to traverse the device path nodes and acquire the
file part by stepping through the nodes of the directory path until the
file is reached.  We already have efi_dp_split_file_path() for that so
rewrite the function and clean it up to use existing code.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_file.c | 52 ++++++++++++---------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index c96a7f7ca371..e1695ba309e4 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -1104,53 +1104,33 @@ static const struct efi_file_handle efi_file_handle_protocol = {
 struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
 {
 	struct efi_simple_file_system_protocol *v;
-	struct efi_file_handle *f;
+	struct efi_device_path_file_path *mdp;
+	struct efi_device_path *dev_dp, *file_dp;
+	struct efi_file_handle *vol_handle, *file_handle = NULL;
 	efi_status_t ret;
 
 	v = efi_fs_from_path(fp);
 	if (!v)
 		return NULL;
 
-	EFI_CALL(ret = v->open_volume(v, &f));
+	ret = efi_dp_split_file_path(fp, &dev_dp, &file_dp);
 	if (ret != EFI_SUCCESS)
 		return NULL;
+	efi_free_pool(dev_dp);
 
-	/* Skip over device-path nodes before the file path. */
-	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
-		fp = efi_dp_next(fp);
-
-	/*
-	 * Step through the nodes of the directory path until the actual file
-	 * node is reached which is the final node in the device path.
-	 */
-	while (fp) {
-		struct efi_device_path_file_path *fdp =
-			container_of(fp, struct efi_device_path_file_path, dp);
-		struct efi_file_handle *f2;
-		u16 *filename;
-
-		if (!EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH)) {
-			printf("bad file path!\n");
-			f->close(f);
-			return NULL;
-		}
-
-		filename = u16_strdup(fdp->str);
-		if (!filename)
-			return NULL;
-		EFI_CALL(ret = f->open(f, &f2, filename,
-				       EFI_FILE_MODE_READ, 0));
-		free(filename);
-		if (ret != EFI_SUCCESS)
-			return NULL;
-
-		fp = efi_dp_next(fp);
-
-		EFI_CALL(f->close(f));
-		f = f2;
+	ret = efi_open_volume_int(v, &vol_handle);
+	if (ret != EFI_SUCCESS) {
+		efi_free_pool(file_dp);
+		return NULL;
 	}
 
-	return f;
+	mdp = (struct efi_device_path_file_path *)file_dp;
+	/* we don't really care about the ret here *file_handle will be NULL */
+	ret = efi_file_open_int(vol_handle, &file_handle, mdp->str, EFI_FILE_MODE_READ, 0);
+	efi_free_pool(file_dp);
+	efi_file_close_int(vol_handle);
+
+	return file_handle;
 }
 
 efi_status_t efi_open_volume_int(struct efi_simple_file_system_protocol *this,
-- 
2.38.1

Re: [PATCH] efi_loader: simplify efi_load_from_path

[Ilias Apalodimas]

Heinrich

Please ignore this.  As we discussed although this cleans up things
considerably,  it's not exactly what the spec describes.  So let's
keep the existing function.

I've sent another patch cleaning up the EFI_CALL() iterations

Thanks
/Ilias

On Thu, 10 Nov 2022 at 13:36, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> The current implementation efi_load_from_path is a bit confusing.
> First of all it tries to check the device path to make sure it contains
> the path to the device plus the media path with the filename and nothing in
> between.  But that should already be valid since U-Boot constructs those
> device paths.
> On top of that it tries to traverse the device path nodes and acquire the
> file part by stepping through the nodes of the directory path until the
> file is reached.  We already have efi_dp_split_file_path() for that so
> rewrite the function and clean it up to use existing code.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  lib/efi_loader/efi_file.c | 52 ++++++++++++---------------------------
>  1 file changed, 16 insertions(+), 36 deletions(-)
>
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index c96a7f7ca371..e1695ba309e4 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -1104,53 +1104,33 @@ static const struct efi_file_handle efi_file_handle_protocol = {
>  struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
>  {
>         struct efi_simple_file_system_protocol *v;
> -       struct efi_file_handle *f;
> +       struct efi_device_path_file_path *mdp;
> +       struct efi_device_path *dev_dp, *file_dp;
> +       struct efi_file_handle *vol_handle, *file_handle = NULL;
>         efi_status_t ret;
>
>         v = efi_fs_from_path(fp);
>         if (!v)
>                 return NULL;
>
> -       EFI_CALL(ret = v->open_volume(v, &f));
> +       ret = efi_dp_split_file_path(fp, &dev_dp, &file_dp);
>         if (ret != EFI_SUCCESS)
>                 return NULL;
> +       efi_free_pool(dev_dp);
>
> -       /* Skip over device-path nodes before the file path. */
> -       while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> -               fp = efi_dp_next(fp);
> -
> -       /*
> -        * Step through the nodes of the directory path until the actual file
> -        * node is reached which is the final node in the device path.
> -        */
> -       while (fp) {
> -               struct efi_device_path_file_path *fdp =
> -                       container_of(fp, struct efi_device_path_file_path, dp);
> -               struct efi_file_handle *f2;
> -               u16 *filename;
> -
> -               if (!EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH)) {
> -                       printf("bad file path!\n");
> -                       f->close(f);
> -                       return NULL;
> -               }
> -
> -               filename = u16_strdup(fdp->str);
> -               if (!filename)
> -                       return NULL;
> -               EFI_CALL(ret = f->open(f, &f2, filename,
> -                                      EFI_FILE_MODE_READ, 0));
> -               free(filename);
> -               if (ret != EFI_SUCCESS)
> -                       return NULL;
> -
> -               fp = efi_dp_next(fp);
> -
> -               EFI_CALL(f->close(f));
> -               f = f2;
> +       ret = efi_open_volume_int(v, &vol_handle);
> +       if (ret != EFI_SUCCESS) {
> +               efi_free_pool(file_dp);
> +               return NULL;
>         }
>
> -       return f;
> +       mdp = (struct efi_device_path_file_path *)file_dp;
> +       /* we don't really care about the ret here *file_handle will be NULL */
> +       ret = efi_file_open_int(vol_handle, &file_handle, mdp->str, EFI_FILE_MODE_READ, 0);
> +       efi_free_pool(file_dp);
> +       efi_file_close_int(vol_handle);
> +
> +       return file_handle;
>  }
>
>  efi_status_t efi_open_volume_int(struct efi_simple_file_system_protocol *this,
> --
> 2.38.1
>