Re: [Patch 2/2] MdeModulePkg: Did some code enhancement for DxeIpIpLib.

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

Patch title: DxeIpIoLib

On 1/10/2018 11:16 AM, Wang Fan wrote:
> * In DxeIpIo, there are several places use ASSERT() to check input
>    parameters without and descriptions or error handling. This patch
>    fixed this issue.
> * Fixed some incorrect descriptions in code commence.
> * Remove unneeded Exit tag in function IpIoOpen and IpIoConfigIp.
> * Add EFIAPI tag for function IpIoRefreshNeighbor.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
>   MdeModulePkg/Include/Library/IpIoLib.h       | 21 +++++++++--
>   MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.c | 52 ++++++++++++++++++----------
>   2 files changed, 52 insertions(+), 21 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/IpIoLib.h b/MdeModulePkg/Include/Library/IpIoLib.h
> index 463bf95..61653b0 100644
> --- a/MdeModulePkg/Include/Library/IpIoLib.h
> +++ b/MdeModulePkg/Include/Library/IpIoLib.h
> @@ -308,10 +308,12 @@ typedef struct _IP_IO_IP_INFO {
>     UINT8                     IpVersion;
>   } IP_IO_IP_INFO;
>   
>   /**
>     Create a new IP_IO instance.
> +
> +  If IpVersion is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     This function uses IP4/IP6 service binding protocol in Controller to create
>     an IP4/IP6 child (aka IP4/IP6 instance).
>   
>     @param[in]  Image             The image handle of the driver or application that
> @@ -351,10 +353,12 @@ IpIoDestroy (
>     IN OUT IP_IO *IpIo
>     );
>   
>   /**
>     Stop an IP_IO instance.
> +
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     This function is paired with IpIoOpen(). The IP_IO will be unconfigured, and all
>     pending send/receive tokens will be canceled.
>   
>     @param[in, out]  IpIo            The pointer to the IP_IO instance that needs to stop.
> @@ -370,11 +374,13 @@ IpIoStop (
>     IN OUT IP_IO *IpIo
>     );
>   
>   /**
>     Open an IP_IO instance for use.
> -
> +
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
> +
>     This function is called after IpIoCreate(). It is used for configuring the IP
>     instance and register the callbacks and their context data for sending and
>     receiving IP packets.
>   
>     @param[in, out]  IpIo               The pointer to an IP_IO instance that needs
> @@ -399,11 +405,11 @@ IpIoOpen (
>     );
>   
>   /**
>     Send out an IP packet.
>     
> -  This function is called after IpIoOpen(). The data to be sent are wrapped in
> +  This function is called after IpIoOpen(). The data to be sent is wrapped in
>     Pkt. The IP instance wrapped in IpIo is used for sending by default but can be
>     overriden by Sender. Other sending configs, like source address and gateway
>     address etc., are specified in OverrideData.
>   
>     @param[in, out]  IpIo                  Pointer to an IP_IO instance used for sending IP
> @@ -437,10 +443,13 @@ IpIoSend (
>     );
>   
>   /**
>     Cancel the IP transmit token that wraps this Packet.
>   
> +  If IpIo is NULL, then ASSERT().
> +  If Packet is NULL, then ASSERT().
> +
>     @param[in]  IpIo                  The pointer to the IP_IO instance.
>     @param[in]  Packet                The pointer to the packet of NET_BUF to cancel.
>   
>   **/
>   VOID
> @@ -450,10 +459,13 @@ IpIoCancelTxToken (
>     IN VOID   *Packet
>     );
>   
>   /**
>     Add a new IP instance for sending data.
> +
> +  If IpIo is NULL, then ASSERT().
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     The function is used to add the IP_IO to the IP_IO sending list. The caller
>     can later use IpIoFindSender() to get the IP_IO and call IpIoSend() to send
>     data.
>   
> @@ -471,10 +483,12 @@ IpIoAddIp (
>   
>   /**
>     Configure the IP instance of this IpInfo and start the receiving if IpConfigData
>     is not NULL.
>   
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
> +
>     @param[in, out]  IpInfo          The pointer to the IP_IO_IP_INFO instance.
>     @param[in, out]  IpConfigData    The IP4 or IP6 configure data used to configure
>                                      the IP instance. If NULL, the IP instance is reset.
>                                      If UseDefaultAddress is set to TRUE, and the configure
>                                      operation succeeds, the default address information
> @@ -493,10 +507,12 @@ IpIoConfigIp (
>     );
>   
>   /**
>     Destroy an IP instance maintained in IpIo->IpList for
>     sending purpose.
> +
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     This function pairs with IpIoAddIp(). The IpInfo is previously created by
>     IpIoAddIp(). The IP_IO_IP_INFO::RefCnt is decremented and the IP instance
>     will be dstroyed if the RefCnt is zero.
>   
> @@ -584,10 +600,11 @@ IpIoGetIcmpErrStatus (
>     @retval      EFI_UNSUPPORTED       IP version is IPv4, which doesn't support neighbor cache refresh.
>     @retval      EFI_OUT_OF_RESOURCES  Failed due to resource limitations.
>   
>   **/
>   EFI_STATUS
> +EFIAPI
>   IpIoRefreshNeighbor (
>     IN IP_IO           *IpIo,
>     IN EFI_IP_ADDRESS  *Neighbor,
>     IN UINT32          Timeout
>     );
> diff --git a/MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.c b/MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.c
> index c7bc1aa..0c6681d 100644
> --- a/MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.c
> +++ b/MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.c
> @@ -1,10 +1,10 @@
>   /** @file
>     IpIo Library.
>   
>   (C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>
> -Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 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
>   http://opensource.org/licenses/bsd-license.php
>   
> @@ -632,12 +632,12 @@ IpIoExtFree (
>     @param[in]       Context              Pointer to the context.
>     @param[in]       NotifyData           Pointer to the notify data.
>     @param[in]       Dest                 Pointer to the destination IP address.
>     @param[in]       Override             Pointer to the overriden IP_IO data.
>   
> -  @return Pointer to the data structure created to wrap the packet. If NULL,
> -  @return resource limit occurred.
> +  @return Pointer to the data structure created to wrap the packet. If any error occurs,
> +          then return NULL.
>   
>   **/
>   IP_IO_SEND_ENTRY *
>   IpIoCreateSndEntry (
>     IN OUT IP_IO             *IpIo,
> @@ -1073,11 +1073,11 @@ IpIoListenHandlerDpc (
>       if ((EFI_IP4 (RxData->Ip4RxData.Header->SourceAddress) != 0) &&
>           (IpIo->SubnetMask != 0) &&
>           IP4_NET_EQUAL (IpIo->StationIp, EFI_NTOHL (((EFI_IP4_RECEIVE_DATA *) RxData)->Header->SourceAddress), IpIo->SubnetMask) &&
>           !NetIp4IsUnicast (EFI_NTOHL (((EFI_IP4_RECEIVE_DATA *) RxData)->Header->SourceAddress), IpIo->SubnetMask)) {
>         //
> -      // The source address is not zero and it's not a unicast IP address, discard it.
> +      // The source address doesn't match StationIp and it's not a unicast IP address, discard it.
>         //
>         goto CleanUp;
>       }
>   
>       if (RxData->Ip4RxData.DataLength == 0) {
> @@ -1219,10 +1219,12 @@ IpIoListenHandler (
>   }
>   
>   
>   /**
>     Create a new IP_IO instance.
> +
> +  If IpVersion is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     This function uses IP4/IP6 service binding protocol in Controller to create
>     an IP4/IP6 child (aka IP4/IP6 instance).
>   
>     @param[in]  Image             The image handle of the driver or application that
> @@ -1284,11 +1286,11 @@ IpIoCreate (
>     Status = IpIoCreateIpChildOpenProtocol (
>                Controller,
>                Image,
>                &IpIo->ChildHandle,
>                IpVersion,
> -             (VOID **)&(IpIo->Ip)
> +             (VOID **) & (IpIo->Ip)
>                );
>     if (EFI_ERROR (Status)) {
>       goto ReleaseIpIo;
>     }
>   
> @@ -1306,11 +1308,13 @@ ReleaseIpIo:
>   }
>   
>   
>   /**
>     Open an IP_IO instance for use.
> -
> +
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
> +
>     This function is called after IpIoCreate(). It is used for configuring the IP
>     instance and register the callbacks and their context data for sending and
>     receiving IP packets.
>   
>     @param[in, out]  IpIo               Pointer to an IP_IO instance that needs
> @@ -1417,11 +1421,11 @@ IpIoOpen (
>                                IpIo->Ip.Ip4,
>                                &(IpIo->RcvToken.Ip4Token)
>                                );
>       if (EFI_ERROR (Status)) {
>         IpIo->Ip.Ip4->Configure (IpIo->Ip.Ip4, NULL);
> -      goto ErrorExit;
> +      return Status;
>       }
>   
>     } else {
>   
>       IpIo->Protocol = OpenData->IpConfigData.Ip6CfgData.DefaultProtocol;
> @@ -1429,25 +1433,25 @@ IpIoOpen (
>                                IpIo->Ip.Ip6,
>                                &(IpIo->RcvToken.Ip6Token)
>                                );
>       if (EFI_ERROR (Status)) {
>         IpIo->Ip.Ip6->Configure (IpIo->Ip.Ip6, NULL);
> -      goto ErrorExit;
> +      return Status;
>       }
>     }
>   
>     IpIo->IsConfigured = TRUE;
>     InsertTailList (&mActiveIpIoList, &IpIo->Entry);
>   
> -ErrorExit:
> -
>     return Status;
>   }
>   
>   
>   /**
>     Stop an IP_IO instance.
> +
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     This function is paired with IpIoOpen(). The IP_IO will be unconfigured and all
>     the pending send/receive tokens will be canceled.
>   
>     @param[in, out]  IpIo            Pointer to the IP_IO instance that needs to stop.
> @@ -1575,11 +1579,11 @@ IpIoDestroy (
>   
>   
>   /**
>     Send out an IP packet.
>     
> -  This function is called after IpIoOpen(). The data to be sent are wrapped in
> +  This function is called after IpIoOpen(). The data to be sent is wrapped in
>     Pkt. The IP instance wrapped in IpIo is used for sending by default but can be
>     overriden by Sender. Other sending configs, like source address and gateway
>     address etc., are specified in OverrideData.
>   
>     @param[in, out]  IpIo                  Pointer to an IP_IO instance used for sending IP
> @@ -1662,10 +1666,13 @@ IpIoSend (
>   
>   
>   /**
>     Cancel the IP transmit token which wraps this Packet.
>   
> +  If IpIo is NULL, then ASSERT().
> +  If Packet is NULL, then ASSERT().
> +
>     @param[in]  IpIo                  Pointer to the IP_IO instance.
>     @param[in]  Packet                Pointer to the packet of NET_BUF to cancel.
>   
>   **/
>   VOID
> @@ -1708,10 +1715,13 @@ IpIoCancelTxToken (
>   }
>   
>   
>   /**
>     Add a new IP instance for sending data.
> +
> +  If IpIo is NULL, then ASSERT().
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     The function is used to add the IP_IO to the IP_IO sending list. The caller
>     can later use IpIoFindSender() to get the IP_IO and call IpIoSend() to send
>     data.
>   
> @@ -1735,10 +1745,11 @@ IpIoAddIp (
>   
>     IpInfo = AllocatePool (sizeof (IP_IO_IP_INFO));
>     if (IpInfo == NULL) {
>       return NULL;
>     }
> +  ASSERT ((IpInfo->IpVersion == IP_VERSION_4) || (IpInfo->IpVersion == IP_VERSION_6));
>   
>     //
>     // Init this IpInfo, set the Addr and SubnetMask to 0 before we configure the IP
>     // instance.
>     //
> @@ -1810,10 +1821,13 @@ ReleaseIpInfo:
>   
>   /**
>     Configure the IP instance of this IpInfo and start the receiving if IpConfigData
>     is not NULL.
>   
> +  If IpInfo is NULL, then ASSERT().
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
> +
>     @param[in, out]  IpInfo          Pointer to the IP_IO_IP_INFO instance.
>     @param[in, out]  IpConfigData    The IP configure data used to configure the IP
>                                      instance, if NULL the IP instance is reset. If
>                                      UseDefaultAddress is set to TRUE, and the configure
>                                      operation succeeds, the default address information
> @@ -1859,11 +1873,11 @@ IpIoConfigIp (
>     } else {
>       Status = Ip.Ip6->Configure (Ip.Ip6, IpConfigData);
>     }
>   
>     if (EFI_ERROR (Status)) {
> -    goto OnExit;
> +    return Status;
>     }
>   
>     if (IpConfigData != NULL) {
>       if (IpInfo->IpVersion == IP_VERSION_4) {
>   
> @@ -1874,11 +1888,11 @@ IpIoConfigIp (
>                              NULL,
>                              NULL
>                              );
>           if (EFI_ERROR (Status)) {
>             Ip.Ip4->Configure (Ip.Ip4, NULL);
> -          goto OnExit;
> +          return Status;
>           }
>   
>           IP4_COPY_ADDRESS (&((EFI_IP4_CONFIG_DATA*) IpConfigData)->StationAddress, &Ip4ModeData.ConfigData.StationAddress);
>           IP4_COPY_ADDRESS (&((EFI_IP4_CONFIG_DATA*) IpConfigData)->SubnetMask, &Ip4ModeData.ConfigData.SubnetMask);
>         }
> @@ -1908,11 +1922,11 @@ IpIoConfigIp (
>                            NULL,
>                            NULL
>                            );
>         if (EFI_ERROR (Status)) {
>           Ip.Ip6->Configure (Ip.Ip6, NULL);
> -        goto OnExit;
> +        return Status;
>         }
>   
>         if (Ip6ModeData.IsConfigured) {
>           CopyMem (
>             &((EFI_IP6_CONFIG_DATA *) IpConfigData)->StationAddress,
> @@ -1944,11 +1958,11 @@ IpIoConfigIp (
>             FreePool (Ip6ModeData.IcmpTypeList);
>           }
>   
>         } else {
>           Status = EFI_NO_MAPPING;
> -        goto OnExit;
> +        return Status;
>         }
>   
>         CopyMem (
>           &IpInfo->Addr,
>           &Ip6ModeData.ConfigData.StationAddress,
> @@ -1969,19 +1983,19 @@ IpIoConfigIp (
>       //
>       ZeroMem (&IpInfo->Addr, sizeof (IpInfo->Addr));
>       ZeroMem (&IpInfo->PreMask, sizeof (IpInfo->PreMask));
>     }
>   
> -OnExit:
> -
>     return Status;
>   }
>   
>   
>   /**
>     Destroy an IP instance maintained in IpIo->IpList for
>     sending purpose.
> +
> +  If Ip version is not IP_VERSION_4 or IP_VERSION_6, then ASSERT().
>     
>     This function pairs with IpIoAddIp(). The IpInfo is previously created by
>     IpIoAddIp(). The IP_IO_IP_INFO::RefCnt is decremented and the IP instance
>     will be dstroyed if the RefCnt is zero.
>   
> @@ -2110,12 +2124,11 @@ IpIoFindSender (
>   
>           if (EFI_IP6_EQUAL (&IpInfo->Addr.v6, &Src->v6)) {
>             *IpIo = IpIoPtr;
>             return IpInfo;
>           }
> -      }
> -
> +      }
>       }
>     }
>   
>     //
>     // No match.
> @@ -2258,10 +2271,11 @@ IpIoGetIcmpErrStatus (
>     @retval      EFI_UNSUPPORTED       IP version is IPv4, which doesn't support neighbor cache refresh.
>     @retval      EFI_OUT_OF_RESOURCES  Failed due to resource limit.
>   
>   **/
>   EFI_STATUS
> +EFIAPI
>   IpIoRefreshNeighbor (
>     IN IP_IO           *IpIo,
>     IN EFI_IP_ADDRESS  *Neighbor,
>     IN UINT32          Timeout
>     )
> 


-- 
Thanks,
Ray