Re: [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

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

Hi Udit,

What does "If mSmmIoLibGcdMemSpace is NULL then if condition is not reachable." mean?

ASSERT_EFI_ERROR macro only effects at DEBUG mode, and the if condition is for error handling.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Udit Kumar
Sent: Tuesday, June 6, 2017 7:11 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check instead of useless Status check

Hi Star 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Star Zeng
> Sent: Monday, June 05, 2017 12:11 PM
> To: edk2-devel@lists.01.org
> Cc: Jiewen Yao <jiewen.yao@intel.com>; Star Zeng <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdePkg SmmIoLib: Use NULL pointer check 
> instead of useless Status check
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=587
> 
> The Status check in "if (!EFI_ERROR (Status))" condition is useless, 
> it should be NULL pointer check. And this patch also fixes a typo 
> "continous" to "continuous".
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdePkg/Library/SmmIoLib/SmmIoLib.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> index 181abb8e25df..f1cb0dace500 100644
> --- a/MdePkg/Library/SmmIoLib/SmmIoLib.c
> +++ b/MdePkg/Library/SmmIoLib/SmmIoLib.c
> @@ -156,7 +156,7 @@ SmmIsMmioValid (
>  }
> 
>  /**
> -  Merge continous entries whose type is EfiGcdMemoryTypeMemoryMappedIo.
> +  Merge continuous entries whose type is
> EfiGcdMemoryTypeMemoryMappedIo.
> 
>    @param[in, out]  GcdMemoryMap           A pointer to the buffer in which
> firmware places
>                                            the current GCD memory map.
> @@ -217,7 +217,8 @@ MergeGcdMmioEntry (
>    @param[in] Interface  Points to the interface instance.
>    @param[in] Handle     The handle on which the interface was installed.
> 
> -  @retval EFI_SUCCESS   Notification runs successfully.
> +  @retval EFI_SUCCESS           Notification runs successfully.
> +  @retval EFI_OUT_OF_RESOURCES  No enough resources to save GCD MMIO
> map.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -237,10 +238,10 @@ SmmIoLibInternalEndOfDxeNotify (
>      MergeGcdMmioEntry (MemSpaceMap, &NumberOfDescriptors);
> 
>      mSmmIoLibGcdMemSpace = AllocateCopyPool (NumberOfDescriptors * 
> sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR), MemSpaceMap);
> -    ASSERT_EFI_ERROR (Status);
> -    if (EFI_ERROR (Status)) {
> +    ASSERT (mSmmIoLibGcdMemSpace != NULL);
> +    if (mSmmIoLibGcdMemSpace == NULL) {


If mSmmIoLibGcdMemSpace is NULL then if condition is not reachable. 
If not NULL then if condition will be false always, I think if condition is not needed.  


>        gBS->FreePool (MemSpaceMap);
> -      return Status;
> +      return EFI_OUT_OF_RESOURCES;
>      }
> 
>      mSmmIoLibGcdMemNumberOfDesc = NumberOfDescriptors;
> --
> 2.7.0.windows.1


Regards
Udit

> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel