Re: [edk2-devel] [PATCH] NetworkPkg: Addressed static code analyzer issues

["Laszlo Ersek" <lersek@redhat.com>]

adding NetworkPkg maintainers, comments below

On 06/18/21 05:30, INDIA\sivaramann wrote:
> Issue on the PxeBcDhcp4CallBack() functions of UEFIPXEBC Driver.
> In this function allowed events are Dhcp4RcvdOffer, Dhcp4SelectOffer,
> Dhcp4SendDiscover, Dhcp4RcvdAck. If any other event comes as input
> it will exit in beginning itself.

Yes.

> 
> Later below switch case handling the default case which is not reachable.
> I assume this code is a not reachable code and can be removed

(1) The edk2 coding style recommends adding "default" cases to switch
statements, as far as I recall. I'd keep the default, but add

  ASSERT (FALSE);

there.

(2) There is a more confusing style issue with the same switch
statement. Namely, it has a case label for "Dhcp4SendRequest". Control
will never jump to that label, due to the "if" at the top of the
function that you highlight.

Importantly, the *code* starting at the "Dhcp4SendRequest" case label
must not be removed, as the "Dhcp4SendDiscover" logic *falls through* to
it. However, the "Dhcp4SendRequest" case label itself should be removed,
and the comment just above it should be updated.

This dead label seems to originate from historical commit a3bcde70e6dc
("Add NetworkPkg (P.UDK2010.UP3.Network.P1)", 2010-11-01).

(3) The subject line is nearly useless, please name at least
"NetworkPkg/UefiPxeBcDxe".

Thanks
Laszlo


> 
> Signed-off-by: Sivaraman <sivaramann@ami.com>
> ---
>  NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> index fb63cf61a9..c0d8211ea0 100644
> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> @@ -1331,8 +1331,6 @@ PxeBcDhcp4CallBack (
>      }
>      break;
>  
> -  default:
> -    break;
>    }
>  
>    return Status;
>