Re: [PATCH v3 3/6] arm64 linux loader: rename functions and macros and move to common headers

[Leif Lindholm]

On Fri, Jul 06, 2018 at 05:28:17PM +0200, Daniel Kiper wrote:
> On Wed, Jun 27, 2018 at 06:17:17PM +0100, Leif Lindholm wrote:
> > In preparation for using the linux loader for 32-bit and 64-bit platforms,
> > rename grub_arm64*/GRUB_ARM64* to grub_efi*/GRUB_EFI*.
> >
> > Move prototypes for now-common functions to efi/efi.h.
> >
> > Signed-off-by: Leif Lindholm <address@hidden>
> > ---
> >  grub-core/loader/arm64/linux.c    | 14 +++++++-------
> >  grub-core/loader/arm64/xen_boot.c | 10 +++++-----
> >  include/grub/arm64/linux.h        |  9 ++++-----
> >  include/grub/efi/efi.h            |  4 ++++
> >  4 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index ebe1e730d..577fbda54 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -48,9 +48,9 @@ static grub_addr_t initrd_start;
> >  static grub_addr_t initrd_end;
> >
> >  grub_err_t
> > -grub_arm64_uefi_check_image (struct linux_arm64_kernel_header * lh)
> > +grub_efi_linux_check_image (struct linux_armxx_kernel_header * lh)
> 
> s/grub_efi_linux_check_image/grub_armxx_efi_linux_check_image/
> 
> This and functions below seems to be ARM specific, so, I would
> add "_armxx_" to the name as above. Same below please.

Yeah, fair point.

> >  {
> > -  if (lh->magic != GRUB_LINUX_ARM64_MAGIC_SIGNATURE)
> > +  if (lh->magic != LINUX_ARMXX_MAGIC_SIGNATURE)
> 
> s/LINUX_ARMXX_MAGIC_SIGNATURE/GRUB_LINUX_ARMXX_MAGIC_SIGNATURE/g
> 
> Hmmm... Why do you drop "GRUB_" prefix?

I stumbled a bit around here tbh.
My rationale was a bit like for struct linux_*_kernel_header instead
of grub_linux_*_kernel_header.

I don't mind changing.

> >      return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> >
> >    if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
> > @@ -109,7 +109,7 @@ failure:
> >  }
> >
> >  grub_err_t
> > -grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> > +grub_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> 
> s/grub_efi_linux_boot_image/grub_armxx_efi_linux_boot_image/g
> 
> ...and below please...

Sure.

/
    Leif