[PATCH v3 1/7] efi_loader: Refactor device and image paths into a function

[Heinrich Schuchardt]

On 12/19/24 03:38, Simon Glass wrote:
> Move this code into a function so it can be called from elsewhere.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> Changes in v3:
> - Make calculate_paths() static and add a comment
>
>   lib/efi_loader/efi_bootbin.c | 85 ++++++++++++++++++++++++------------
>   1 file changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
> index b677bbc3124..5016ba7e225 100644
> --- a/lib/efi_loader/efi_bootbin.c
> +++ b/lib/efi_loader/efi_bootbin.c
> @@ -44,12 +44,64 @@ void efi_clear_bootdev(void)
>   	image_size = 0;
>   }
>
> +/**
> + * calculate_paths() - Calculate the device and image patch given a device

%s/patch/path/

The description makes no sense. You cannot derive an image device path
from a device.

> + *
> + * @dev:		device, e.g. "MMC"
> + * @devnr:		number of the device, e.g. "1:2"
> + * @path:		path to file loaded
> + * @device_pathp:	returns EFI device path
> + * @image_pathp:	returns EFI image path
> + * Return EFI_SUCCESS on success, else error code

This would not work with Sphinx

%s/Return/Return:/

> + */
> +static efi_status_t calculate_paths(const char *dev, const char *devnr,
> +				    const char *path,
> +				    struct efi_device_path **device_pathp,
> +				    struct efi_device_path **image_pathp)
> +{
> +	struct efi_device_path *image, *device;
> +	efi_status_t ret;
> +
> +#if IS_ENABLED(CONFIG_NETDEVICES)

Please, avoid #if. The include needs to be fixed.

> +	if (!strcmp(dev, "Net") || !strcmp(dev, "Http")) {
> +		ret = efi_net_set_dp(dev, devnr);
> +		if (ret != EFI_SUCCESS)
> +			return ret;
> +	}

This does not fit to the function description. It has nothing to do with
calculating device-paths.

> +#endif
> +
> +	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	*device_pathp = device;
> +	if (image) {
> +		/* FIXME: image should not contain device */

We could change efi_dp_from_name() but then we would have to concatenate
paths in create_lo_dp_part().

This does not deserve a FIXME label.

Best regards

Heinrich

> +		struct efi_device_path *image_tmp = image;
> +
> +		efi_dp_split_file_path(image, &device, &image);
> +		efi_free_pool(image_tmp);
> +	}
> +	*image_pathp = image;
> +	log_debug("- boot device %pD\n", device);
> +	if (image)
> +		log_debug("- image %pD\n", image);
> +
> +	return EFI_SUCCESS;
> +}
> +
>   /**
>    * efi_set_bootdev() - set boot device
>    *
>    * This function is called when a file is loaded, e.g. via the 'load' command.
>    * We use the path to this file to inform the UEFI binary about the boot device.
>    *
> + * For a valid image, it sets:
> + *    - image_addr to the provided buffer
> + *    - image_size to the provided buffer_size
> + *    - bootefi_device_path to the EFI device-path
> + *    - bootefi_image_path to the EFI image-path
> + *
>    * @dev:		device, e.g. "MMC"
>    * @devnr:		number of the device, e.g. "1:2"
>    * @path:		path to file loaded
> @@ -59,7 +111,6 @@ void efi_clear_bootdev(void)
>   void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
>   		     void *buffer, size_t buffer_size)
>   {
> -	struct efi_device_path *device, *image;
>   	efi_status_t ret;
>
>   	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
> @@ -93,34 +144,12 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
>   	image_addr = buffer;
>   	image_size = buffer_size;
>
> -#if IS_ENABLED(CONFIG_NETDEVICES)
> -	if (!strcmp(dev, "Net") || !strcmp(dev, "Http")) {
> -		ret = efi_net_set_dp(dev, devnr);
> -		if (ret != EFI_SUCCESS)
> -			goto error;
> -	}
> -#endif
> -
> -	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> -	if (ret != EFI_SUCCESS)
> -		goto error;
> -
> -	bootefi_device_path = device;
> -	if (image) {
> -		/* FIXME: image should not contain device */
> -		struct efi_device_path *image_tmp = image;
> -
> -		efi_dp_split_file_path(image, &device, &image);
> -		efi_free_pool(image_tmp);
> +	ret = calculate_paths(dev, devnr, path, &bootefi_device_path,
> +			      &bootefi_image_path);
> +	if (ret) {
> +		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
> +		efi_clear_bootdev();
>   	}
> -	bootefi_image_path = image;
> -	log_debug("- boot device %pD\n", device);
> -	if (image)
> -		log_debug("- image %pD\n", image);
> -	return;
> -error:
> -	log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
> -	efi_clear_bootdev();
>   }
>
>   /**