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

["Fu, Siyuan" <siyuan.fu@intel.com>]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>



> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Wang Fan
> Sent: Wednesday, January 10, 2018 11:16 AM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Subject: [edk2] [Patch 2/2] MdeModulePkg: Did some code enhancement for
> DxeIpIpLib.
> 
> * 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
>    )
> --
> 1.9.5.msysgit.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel