Re: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when DpcEntry is NULL

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

> -----Original Message-----
> From: Zhang, Shenglei <shenglei.zhang@intel.com>
> Sent: 2019年10月12日 15:43
> To: devel@edk2.groups.io
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [PATCH] NetworkPkg/DpcDxe: Update the consequent logic when
> DpcEntry is NULL
> 
> If DpcEntry is NULL, it means it failed to be allocated space.
> ReturnStatus should be EFI_OUT_OF_RESOURCES regardless of the
> content of mDpcEntryFreeList.
> 
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> ---
>  NetworkPkg/DpcDxe/Dpc.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/NetworkPkg/DpcDxe/Dpc.c b/NetworkPkg/DpcDxe/Dpc.c
> index 8a490949dc8c..ebdb978b254a 100644
> --- a/NetworkPkg/DpcDxe/Dpc.c
> +++ b/NetworkPkg/DpcDxe/Dpc.c
> @@ -137,14 +137,11 @@ DpcQueueDpc (
>        gBS->RaiseTPL (TPL_HIGH_LEVEL);
> 
>        //
> -      // If the allocation of a DPC entry fails, and the free list is empty,
> -      // then return EFI_OUT_OF_RESOURCES.
> +      // If the allocation of a DPC entry fails, return EFI_OUT_OF_RESOURCES.
>        //
>        if (DpcEntry == NULL) {
> -        if (IsListEmpty (&mDpcEntryFreeList)) {
> -          ReturnStatus = EFI_OUT_OF_RESOURCES;
> -          goto Done;
> -        }
> +        ReturnStatus = EFI_OUT_OF_RESOURCES;
> +        goto Done;

I don't think it's a correct fix. This DpcEntry allocation is inside a for loop which
tries to allocate 64 new DPC entries, if one of the allocation in the middle of
this loop failed, the mDpcEntryFreeList will contain useable entries for this
DpcQueueDpc(), that's why original code doesn't treat it as an error.

How do you find this issue? By code review or it cause some real problems?

Thanks
Siyuan

>        }
> 
>        //
> --
> 2.18.0.windows.1