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

[Simon Glass]

Hi Heinrich,

On Sun, 22 Dec 2024 at 05:35, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.

I'll change it to 'strings'

>
> > + *
> > + * @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.

That code was added just recently, reviewed by you, so I will let you
clean it up. It is perpetuating the globals that this series is (was)
resolving.

Anyway, the naming is correct, once that is fixed.

>
> > +#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.

This is copied from the existing code. Please check the patch before
requesting unrelated changes.

>
> 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();
> >   }
> >
> >   /**
>

Regards,
Simon