Re: [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

["Zeng, Star" <star.zeng@intel.com>]

Mike,

Depend on whether the allocated buffer will be returned to caller or not, and whether the implementation is matched with the function comments.
Some cases seem ok.
1. There are paired interfaces, for example AddUnicodeString and FreeUnicodeStringTable, etc.
2. There are clear comments, and implementation matches with comments, for example GetVariable2, etc.
" The returned buffer is allocated using AllocatePool().  The caller is responsible for freeing this buffer with FreePool(). "

More evaluation are needed for all the interfaces.


Thanks,
Star
-----Original Message-----
From: Kinney, Michael D 
Sent: Thursday, February 1, 2018 10:07 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [edk2] [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

Star,

I agree with your observation.  And the API specifically says that EFI_BOOT_SERVICES.AllocatePool() is used, so I agree with your suggested change.

However, there are other APIs in the UefiLib implementation that use the MemoryAllocationLib services.  Should those be updated too?

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, January 31, 2018 5:32 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2- 
> devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [edk2] [Patch 1/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> 
> Hi Mike,
> 
> UefiLib is capable to be used by SMM Core or SMM driver before 
> SmmReadyToLock.
> 
> And the new interface has the comment below.
> 
> +                            The returned buffer is
> allocated using
> +
> EFI_BOOT_SERVICES.AllocatePool().  The caller is
> +                            responsible for freeing this
> buffer with
> +
> EFI_BOOT_SERVICES.FreePool().
> 
> But the implementation is using AllocateZeroPool() like below, that 
> will direct to gSmst->AllocatePool for SMM Core or SMM driver.
> 
> +  //
> +  // Allocate array of protocol instances  //  *Buffer = 
> + AllocateZeroPool (NoHandles * sizeof (VOID
> *));
> +  if (*Buffer == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> 
> Should the implementation use gBS->AllocatePool() +
> ZeroMem() instead of AllocateZeroPool()?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of Kinney, Michael D
> Sent: Thursday, February 1, 2018 7:33 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2] [Patch 1/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> 
> From: Michael D Kinney <michael.d.kinney@intel.com>
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=838
> 
> Add new API to the UefiLib that locates and returns an array of 
> protocols instances that match a given protocol.
> 
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> ---
>  MdePkg/Include/Library/UefiLib.h |  32 +++++++++++-  
> MdePkg/Library/UefiLib/UefiLib.c | 107
> ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h
> b/MdePkg/Include/Library/UefiLib.h
> index 0b14792a0a..54bc2cc5a3 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -12,7 +12,7 @@
>    of size reduction when compiler optimization is disabled. If 
> MDEPKG_NDEBUG is
>    defined, then debug and assert related macros wrapped by it are the 
> NULL implementations.
> 
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights 
> reserved.<BR>
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights
> reserved.<BR>
>  This program and the accompanying materials are licensed and made 
> available under  the terms and conditions of the BSD License that 
> accompanies this distribution.
>  The full text of the license may be found at @@ -1490,4 +1490,34 @@ 
> CatSPrint (
>    ...
>    );
> 
> +/**
> +  Returns an array of protocol instance that matches the
> given protocol.
> +
> +  @param[in]  Protocol      Provides the protocol to
> search for.
> +  @param[out] NoProtocols   The number of protocols
> returned in Buffer.
> +  @param[out] Buffer        A pointer to the buffer to
> return the requested
> +                            array of protocol instances
> that match Protocol.
> +                            The returned buffer is
> allocated using
> +
> EFI_BOOT_SERVICES.AllocatePool().  The caller is
> +                            responsible for freeing this
> buffer with
> +
> EFI_BOOT_SERVICES.FreePool().
> +
> +  @retval EFI_SUCCESS            The array of protocols
> was returned in Buffer,
> +                                 and the number of
> protocols in Buffer was
> +                                 returned in
> NoProtocols.
> +  @retval EFI_NOT_FOUND          No protocols found.
> +  @retval EFI_OUT_OF_RESOURCES   There is not enough
> pool memory to store the
> +                                 matching results.
> +  @retval EFI_INVALID_PARAMETER  Protocol is NULL.
> +  @retval EFI_INVALID_PARAMETER  NoProtocols is NULL.
> +  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +EfiLocateProtocolBuffer (
> +  IN  EFI_GUID  *Protocol,
> +  OUT UINTN     *NoProtocols,
> +  OUT VOID      ***Buffer
> +  );
>  #endif
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
> b/MdePkg/Library/UefiLib/UefiLib.c
> index a7eee01240..d86db4bd67 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -5,7 +5,7 @@
>    EFI Driver Model related protocols, manage Unicode string tables 
> for UEFI Drivers,
>    and print messages on the console output and standard error 
> devices.
> 
> -  Copyright (c) 2006 - 2017, Intel Corporation. All rights 
> reserved.<BR>
> +  Copyright (c) 2006 - 2018, Intel Corporation. All
> rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of 
> the BSD License
>    which accompanies this distribution.  The full text of the license 
> may be found at @@ -1605,3 +1605,108 @@ GetBestLanguage (
>    //
>    return NULL;
>  }
> +
> +/**
> +  Returns an array of protocol instance that matches the
> given protocol.
> +
> +  @param[in]  Protocol      Provides the protocol to
> search for.
> +  @param[out] NoProtocols   The number of protocols
> returned in Buffer.
> +  @param[out] Buffer        A pointer to the buffer to
> return the requested
> +                            array of protocol instances
> that match Protocol.
> +                            The returned buffer is
> allocated using
> +
> EFI_BOOT_SERVICES.AllocatePool().  The caller is
> +                            responsible for freeing this
> buffer with
> +
> EFI_BOOT_SERVICES.FreePool().
> +
> +  @retval EFI_SUCCESS            The array of protocols
> was returned in Buffer,
> +                                 and the number of
> protocols in Buffer was
> +                                 returned in
> NoProtocols.
> +  @retval EFI_NOT_FOUND          No protocols found.
> +  @retval EFI_OUT_OF_RESOURCES   There is not enough
> pool memory to store the
> +                                 matching results.
> +  @retval EFI_INVALID_PARAMETER  Protocol is NULL.
> +  @retval EFI_INVALID_PARAMETER  NoProtocols is NULL.
> +  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +EfiLocateProtocolBuffer (
> +  IN  EFI_GUID  *Protocol,
> +  OUT UINTN     *NoProtocols,
> +  OUT VOID      ***Buffer
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINTN       NoHandles;
> +  EFI_HANDLE  *HandleBuffer;
> +  UINTN       Index;
> +
> +  //
> +  // Check input parameters
> +  //
> +  if (Protocol == NULL || NoProtocols == NULL || Buffer
> == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Initialze output parameters
> +  //
> +  *NoProtocols = 0;
> +  *Buffer = NULL;
> +
> +  //
> +  // Retrieve the array of handles that support Protocol  //  Status 
> + = gBS->LocateHandleBuffer (
> +                  ByProtocol,
> +                  Protocol,
> +                  NULL,
> +                  &NoHandles,
> +                  &HandleBuffer
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // Allocate array of protocol instances  //  *Buffer = 
> + AllocateZeroPool (NoHandles * sizeof (VOID
> *));
> +  if (*Buffer == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Lookup Protocol on each handle in HandleBuffer to
> fill in the array of
> +  // protocol instances.  Handle case where protocol
> instance was present when
> +  // LocateHandleBuffer() was called, but is not present
> when HandleProtocol()
> +  // is called.
> +  //
> +  for (Index = 0, *NoProtocols = 0; Index < NoHandles;
> Index++) {
> +    Status = gBS->HandleProtocol (
> +                    HandleBuffer[Index],
> +                    Protocol,
> +                    &((*Buffer)[*NoProtocols])
> +                    );
> +    if (!EFI_ERROR (Status)) {
> +      (*NoProtocols)++;
> +    }
> +  }
> +
> +  //
> +  // Free the handle buffer
> +  //
> +  FreePool (HandleBuffer);
> +
> +  //
> +  // Make sure at least one protocol instance was found  //  if 
> + (*NoProtocols == 0) {
> +    FreePool (*Buffer);
> +    *Buffer = NULL;
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> --
> 2.14.2.windows.3
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel