[PATCH v2 4/7] efi_loader: Add a version of efi_binary_run() with more parameters

[Ilias Apalodimas]

Hi Simon,

On Fri, 13 Dec 2024 at 05:53, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 12 Dec 2024 at 08:45, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > On Thu, 12 Dec 2024 at 15:45, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > On Thu, 12 Dec 2024 at 00:38, Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > This uses a few global variables at present. With the bootflow we have
> > > > > the required parameters, so add a function which accepts these. Update
> > > > > the existing function to call the new one with the globals.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >
> > > > > (no changes since v1)
> > > > >
> > > > >  lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++----
> > > > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
> > > > > index 8febd325f34..8ebd48547cb 100644
> > > > > --- a/lib/efi_loader/efi_bootbin.c
> > > > > +++ b/lib/efi_loader/efi_bootbin.c
> > > > > @@ -209,18 +209,22 @@ out:
> > > > >  }
> > > > >
> > > > >  /**
> > > > > - * efi_binary_run() - run loaded UEFI image
> > > > > + * efi_binary_run_() - run loaded UEFI image
> > > > >   *
> > > > >   * @image_ptr: memory address of the UEFI image
> > > > >   * @size:      size of the UEFI image
> > > > >   * @fdt:       device-tree
> > > > > + * @device:    EFI device-path
> > > > > + * @image:     EFI image-path
> > > > >   *
> > > > >   * Execute an EFI binary image loaded at @image.
> > > > >   * @size may be zero if the binary is loaded with U-Boot load command.
> > > > >   *
> > > > >   * Return:     status code
> > > > >   */
> > > > > -efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt)
> > > > > +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
> > > > > +                            struct efi_device_path *device,
> > > > > +                            struct efi_device_path *image)
> > > > >  {
> > > >
> > > > This needs to be static here. Not the next patch. But I don't think we
> > > > need this at all (look below)
> > > >
> > > > >         efi_status_t ret;
> > > > >
> > > > > @@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt)
> > > > >         if (ret != EFI_SUCCESS)
> > > > >                 return ret;
> > > > >
> > > > > -       return efi_run_image(image_ptr, size, bootefi_device_path,
> > > > > -                            bootefi_image_path);
> > > > > +       return efi_run_image(image_ptr, size, device, image);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * efi_binary_run() - run loaded UEFI image
> > > > > + *
> > > > > + * @image:     memory address of the UEFI image
> > > > > + * @size:      size of the UEFI image
> > > > > + * @fdt:       device-tree
> > > > > + *
> > > > > + * Execute an EFI binary image loaded at @image.
> > > > > + * @size may be zero if the binary is loaded with U-Boot load command.
> > > > > + *
> > > > > + * Return:     status code
> > > > > + */
> > > > > +efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> > > > > +{
> > > > > +       return efi_binary_run_(image, size, fdt, bootefi_device_path,
> > > > > +                              bootefi_image_path);
> > > > >  }
> > > >
> > > > We don't need functions and wrappers for such a simple call. Just
> > > > update all the callisites with the extra parameters and keep one
> > > > function
> > >
> > > If you look at the two call sites, you'll see that they would need to
> > > pass bootefi_device_path and bootefi_image_path in. but these are
> > > static variables in this file.
> >
> > Ah fair enough, then please rename efi_binary_run_ -> _efi_binary_run
> > ands make it static on this patch
>
> I don't mind where the underscore goes. I changed my mind about that
> when the dtc maintainer pointed out that C-library functions start
> with underscore, so putting an underscore at the start of the function
> is confusing things.
>
> "The underscore prefix is reserved for functions and types used by the
> compiler and standard library. The standard library can use these
> names freely."[1]
>
> But I don't mind. We don't even use the C library except for sandbox
> and tools. So let me know what you think.

I don't mind really. you can do __efi_binary_run() or keep whjat you
have. I jsut find the leading underscores a bit easier to read and
spot, but not a huge deal

Thanks
/Ilias
>
> Regards,
> SImon
>
> [1] https://stackoverflow.com/questions/39625352/why-do-some-functions-in-c-have-an-underscore-prefix