Re: [PATCH 2/7] efi: refactor grub_efi_allocate_pages

[Leif Lindholm]

On Thu, Jul 27, 2017 at 04:46:14PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:36PM +0100, Leif Lindholm wrote:
> > Expose a new function, grub_efi_allocate_pages_real(), making it possible
> > to specify allocation type and memory type as supported by the UEFI
> > AllocatePages boot service.
> >
> > Make grub_efi_allocate_pages() a consumer of the new function,
> > maintaining its old functionality.
> >
> > Also delete some left-around #if 1/#else blocks in the affected
> > functions.
> >
> > Signed-off-by: Leif Lindholm <address@hidden>
> > ---
> >  grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------
> >  include/grub/efi/efi.h  |  5 +++++
> >  2 files changed, 29 insertions(+), 22 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 460a4b763..7b1763bc5 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;
> >
> >  /* Allocate pages. Return the pointer to the first of allocated pages.  */
> >  void *
> > -grub_efi_allocate_pages (grub_efi_physical_address_t address,
> > -                    grub_efi_uintn_t pages)
> > +grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
> 
> Could not you just add an extra argument to grub_efi_allocate_pages()?
> It is not called in many places. Do you really need separate
> grub_efi_allocate_pages_real()?

Need? No.

It gave an excuse to break some of the implicit semantics of the
existing function out separately from the bit that actually allocates
memory.

I also instinctively dislike functions with > 4 arguments due to
32-bit ARM ABI :)

/
    Leif