Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove unused function arguments

["Dandan Bi" <dandan.bi@intel.com>]

Hi Ard,


Some minor comments,
1. Could you please also remove the arguments in related function comments?
2. Please make the code aligned after removing some condition check.


Thanks,
Dandan
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Friday, April 10, 2020 12:10 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@arm.com>
> Subject: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore/Image: remove
> unused function arguments
> 
> Image.c defines a CoreLoadImageCommon() function that has some
> arguments that are marked OPTIONAL. The function only has a single caller
> living in the same file, and it does not pass any arguments for these
> OPTIONAL arguments.
> 
> So let's simplify the code, and remove these arguments and all the transitive
> conditional dependencies on them.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> 
> NOTE: this patch was generated with the -w option, to suppress indentation
> changes from cluttering the diff.
> 
>  MdeModulePkg/Core/Dxe/Image/Image.c | 81 ++------------------
>  1 file changed, 7 insertions(+), 74 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index aae2c1eaa516..9fdde87f2df3 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -564,13 +564,10 @@ CoreLoadPeImage (
>    IN BOOLEAN                     BootPolicy,
>    IN VOID                        *Pe32Handle,
>    IN LOADED_IMAGE_PRIVATE_DATA   *Image,
> -  IN EFI_PHYSICAL_ADDRESS        DstBuffer    OPTIONAL,
> -  OUT EFI_PHYSICAL_ADDRESS       *EntryPoint  OPTIONAL,
>    IN  UINT32                     Attribute
>    )
>  {
>    EFI_STATUS                Status;
> -  BOOLEAN                   DstBufAlocated;
>    UINTN                     Size;
> 
>    ZeroMem (&Image->ImageContext, sizeof (Image->ImageContext)); @@ -
> 619,15 +616,6 @@ CoreLoadPeImage (
>      return EFI_UNSUPPORTED;
>    }
> 
> -  //
> -  // Allocate memory of the correct memory type aligned on the required
> image boundary
> -  //
> -  DstBufAlocated = FALSE;
> -  if (DstBuffer == 0) {
> -    //
> -    // Allocate Destination Buffer as caller did not pass it in
> -    //
> -
>    if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
>      Size = (UINTN)Image->ImageContext.ImageSize + Image-
> >ImageContext.SectionAlignment;
>    } else {
> @@ -685,31 +673,6 @@ CoreLoadPeImage (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -    DstBufAlocated = TRUE;
> -  } else {
> -    //
> -    // Caller provided the destination buffer
> -    //
> -
> -    if (Image->ImageContext.RelocationsStripped && (Image-
> >ImageContext.ImageAddress != DstBuffer)) {
> -      //
> -      // If the image relocations were stripped, and the caller provided a
> -      // destination buffer address that does not match the address that the
> -      // image is linked at, then the image cannot be loaded.
> -      //
> -      return EFI_INVALID_PARAMETER;
> -    }
> -
> -    if (Image->NumberOfPages != 0 &&
> -        Image->NumberOfPages <
> -        (EFI_SIZE_TO_PAGES ((UINTN)Image->ImageContext.ImageSize +
> Image->ImageContext.SectionAlignment))) {
> -      Image->NumberOfPages = EFI_SIZE_TO_PAGES ((UINTN)Image-
> >ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
> -      return EFI_BUFFER_TOO_SMALL;
> -    }
> -
> -    Image->NumberOfPages = EFI_SIZE_TO_PAGES ((UINTN)Image-
> >ImageContext.ImageSize + Image->ImageContext.SectionAlignment);
> -    Image->ImageContext.ImageAddress = DstBuffer;
> -  }
> 
>    Image->ImageBasePage = Image->ImageContext.ImageAddress;
>    if (!Image->ImageContext.IsTeImage) { @@ -790,13 +753,6 @@
> CoreLoadPeImage (
>      }
>    }
> 
> -  //
> -  // Fill in the entry point of the image if it is available
> -  //
> -  if (EntryPoint != NULL) {
> -    *EntryPoint = Image->ImageContext.EntryPoint;
> -  }
> -
>    //
>    // Print the load address and the PDB file name if it is available
>    //
> @@ -861,11 +817,9 @@ CoreLoadPeImage (
>    // Free memory.
>    //
> 
> -  if (DstBufAlocated) {
>    CoreFreePages (Image->ImageContext.ImageAddress, Image-
> >NumberOfPages);
>    Image->ImageContext.ImageAddress = 0;
>    Image->ImageBasePage = 0;
> -  }
> 
>    if (Image->ImageContext.FixupData != NULL) {
>      CoreFreePool (Image->ImageContext.FixupData); @@ -920,8 +874,7 @@
> CoreLoadedImageInfo (  STATIC  VOID  CoreUnloadAndCloseImage (
> -  IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> -  IN BOOLEAN                    FreePage
> +  IN LOADED_IMAGE_PRIVATE_DATA  *Image
>    )
>  {
>    EFI_STATUS                          Status;
> @@ -1047,7 +1000,7 @@ CoreUnloadAndCloseImage (
>    //
>    // Free the Image from memory
>    //
> -  if ((Image->ImageBasePage != 0) && FreePage) {
> +  if ((Image->ImageBasePage != 0)) {
>      CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
>    }
> 
> @@ -1122,10 +1075,7 @@ CoreLoadImageCommon (
>    IN  EFI_DEVICE_PATH_PROTOCOL         *FilePath,
>    IN  VOID                             *SourceBuffer       OPTIONAL,
>    IN  UINTN                            SourceSize,
> -  IN  EFI_PHYSICAL_ADDRESS             DstBuffer           OPTIONAL,
> -  IN OUT UINTN                         *NumberOfPages      OPTIONAL,
>    OUT EFI_HANDLE                       *ImageHandle,
> -  OUT EFI_PHYSICAL_ADDRESS             *EntryPoint         OPTIONAL,
>    IN  UINT32                           Attribute
>    )
>  {
> @@ -1330,12 +1280,7 @@ CoreLoadImageCommon (
>    Image->Info.FilePath     = DuplicateDevicePath (FilePath);
>    Image->Info.ParentHandle = ParentImageHandle;
> 
> -
> -  if (NumberOfPages != NULL) {
> -    Image->NumberOfPages = *NumberOfPages ;
> -  } else {
>    Image->NumberOfPages = 0 ;
> -  }
> 
>    //
>    // Install the protocol interfaces for this image @@ -1355,20 +1300,11 @@
> CoreLoadImageCommon (
>    //
>    // Load the image.  If EntryPoint is Null, it will not be set.
>    //
> -  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, DstBuffer,
> EntryPoint, Attribute);
> +  Status = CoreLoadPeImage (BootPolicy, &FHand, Image, Attribute);
>    if (EFI_ERROR (Status)) {
> -    if ((Status == EFI_BUFFER_TOO_SMALL) || (Status ==
> EFI_OUT_OF_RESOURCES)) {
> -      if (NumberOfPages != NULL) {
> -        *NumberOfPages = Image->NumberOfPages;
> -      }
> -    }
>      goto Done;
>    }
> 
> -  if (NumberOfPages != NULL) {
> -    *NumberOfPages = Image->NumberOfPages;
> -  }
> -
>    //
>    // Register the image in the Debug Image Info Table if the attribute is set
>    //
> @@ -1448,7 +1384,7 @@ CoreLoadImageCommon (
>    //
>    if (EFI_ERROR (Status)) {
>      if (Image != NULL) {
> -      CoreUnloadAndCloseImage (Image, (BOOLEAN)(DstBuffer == 0));
> +      CoreUnloadAndCloseImage (Image);
>        Image = NULL;
>      }
>    } else if (EFI_ERROR (SecurityStatus)) { @@ -1524,10 +1460,7 @@
> CoreLoadImage (
>               FilePath,
>               SourceBuffer,
>               SourceSize,
> -             (EFI_PHYSICAL_ADDRESS) (UINTN) NULL,
> -             NULL,
>               ImageHandle,
> -             NULL,
>               EFI_LOAD_PE_IMAGE_ATTRIBUTE_RUNTIME_REGISTRATION |
> EFI_LOAD_PE_IMAGE_ATTRIBUTE_DEBUG_IMAGE_INFO_TABLE_REGISTRA
> TION
>               );
> 
> @@ -1744,7 +1677,7 @@ CoreStartImage (
>    // unload it
>    //
>    if (EFI_ERROR (Image->Status) || Image->Type ==
> EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> -    CoreUnloadAndCloseImage (Image, TRUE);
> +    CoreUnloadAndCloseImage (Image);
>      //
>      // ImageHandle may be invalid after the image is unloaded, so use NULL
> handle to record perf log.
>      //
> @@ -1809,7 +1742,7 @@ CoreExit (
>      //
>      // The image has not been started so just free its resources
>      //
> -    CoreUnloadAndCloseImage (Image, TRUE);
> +    CoreUnloadAndCloseImage (Image);
>      Status = EFI_SUCCESS;
>      goto Done;
>    }
> @@ -1911,7 +1844,7 @@ CoreUnloadImage (
>      //
>      // if the Image was not started or Unloaded O.K. then clean up
>      //
> -    CoreUnloadAndCloseImage (Image, TRUE);
> +    CoreUnloadAndCloseImage (Image);
>    }
> 
>  Done:
> --
> 2.17.1
> 
> 
>