Re: [PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Trammell Hudson <hudson@trmm.net>
Cc: <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()
Date: Mon, 14 Sep 2020 11:05:57 +0200	[thread overview]
Message-ID: <20200914090557.GB753@Air-de-Roger> (raw)
In-Reply-To: <20200907190027.669086-3-hudson@trmm.net>

Thanks! Being picky you likely wan to split this into two separate
commits: one for adding need_to_free and the other for
display_file_info.  There's no relation between the two that would
require them to be on the same commit.

On Mon, Sep 07, 2020 at 03:00:25PM -0400, Trammell Hudson wrote:
> From: Trammell hudson <hudson@trmm.net>
> 
> Signed-off-by: Trammell hudson <hudson@trmm.net>
> ---
>  xen/common/efi/boot.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 4022a672c9..f5bdc4b1df 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -102,6 +102,7 @@ union string {
>  
>  struct file {
>      UINTN size;
> +    bool need_to_free;
>      union {
>          EFI_PHYSICAL_ADDRESS addr;
>          void *ptr;
> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
>      if ( !efi_bs )
>          efi_arch_halt();
>  
> -    if ( cfg.addr )
> +    if ( cfg.addr && cfg.need_to_free )
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> -    if ( kernel.addr )
> +    if ( kernel.addr && kernel.need_to_free )
>          efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
> -    if ( ramdisk.addr )
> +    if ( ramdisk.addr && ramdisk.need_to_free )
>          efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
> -    if ( xsm.addr )
> +    if ( xsm.addr && xsm.need_to_free )
>          efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
>  
>      efi_arch_blexit();
> @@ -538,6 +539,21 @@ static char * __init split_string(char *s)
>      return NULL;
>  }
>  
> +static void __init display_file_info(CHAR16 *name, struct file *file, char *options)

I think name at least could be constified?

Also efi_arch_handle_module seem to do more than just printing file
info, hence I would likely rename this to handle_file_info to be more
representative of what it does.

Roger.