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

[Sivaraman Nainar]

Hi Lazlo:

  case Dhcp4SendRequest:
    if (Packet->Length > PXEBC_DHCP4_PACKET_MAX_SIZE) {
      //
      // If the to be sent packet exceeds the maximum length, abort the DHCP process.
      //
      Status = EFI_ABORTED;
      break;
    }
The same packet Length check already taken care on case Dhcp4SendDiscover: also.

So I prefer to remove the above set of lines.

Wil update the subject when sending V2 changes.

-Siva
-----Original Message-----
From: Laszlo Ersek <lersek at redhat.com>
Sent: Wednesday, June 23, 2021 3:45 PM
To: devel at edk2.groups.io; emergingsiva at gmail.com
Cc: Sivaraman Nainar <sivaramann at ami.com>; Maciej Rabeda <maciej.rabeda at linux.intel.com>; Jiaxin Wu <jiaxin.wu at intel.com>; Siyuan Fu <siyuan.fu at intel.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] NetworkPkg: Addressed static code analyzer issues


**CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**

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 at 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;
>

-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76966): https://edk2.groups.io/g/devel/message/76966
Mute This Topic: https://groups.io/mt/83735967/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-