public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: Ranbir Singh <rsingh@ventanamicro.com>
Cc: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"lersek@redhat.com" <lersek@redhat.com>,
	"Ni, Ray" <ray.ni@intel.com>,
	Veeresh Sangolli <veeresh.sangolli@dellteam.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
Date: Wed, 8 Nov 2023 04:05:01 +0000	[thread overview]
Message-ID: <SA2PR11MB49388C38F197FBBAB538594BD2A8A@SA2PR11MB4938.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAA9DWXAfqDkXNmr8niEUCUekpGfLrc9Bd+tht6dJQ+U-ALsNvg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 5460 bytes --]

Hi Ranbir,

Ignoring false positive in static analysis tools is typically a burden.

It is better to avoid false positives with code changes as long as the code changes do not make the implementation confusing and hard to maintain.

I think depending on fall through case statements is confusing and removing them will make the code easier to understand and maintain.

Mike

From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Tuesday, November 7, 2023 7:51 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Subject: Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

As mentioned in the commit message, the comment helps in making it explicit and evident that the missing break is not a human miss, but intentional.
Hence, the comment should be considered as being added for the human code readers / developers.

So even if some static analysis tool flags such an issue, it can be fairly easy now to ignore that on manual inspection. If desired this can also be stated in the comment itself like -

+                  //
+                  // No break here as this is an intentional fall through.
+                  // Ignore any static tool issue if pointed.
+                  //

Yes, there can be other solutions (which may or may not be worth the effort), but for now I went with the least code change approach.

On Tue, Nov 7, 2023 at 11:29 PM Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:
This comment style only works with Coverity.

Other static analysis tools may flag the same issue again.

It is better to update the logic so no static analysis tool will
flag this issue.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 7, 2023 8:23 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>
> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Veeresh Sangolli
> <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>
> Subject: Re: [edk2-devel] [PATCH v2 2/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
>
> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> >
> > The function UpdatePciInfo has switch-case code in which there are
> fall
> > through from case 32: to case 64:. While this is seeemingly
> intentional,
> > it is not evident to any general code reader why there is no break;
> in
> > between. Adding
> >
> >     // No break; here as this is an intentional fallthrough.
> >
> > as comment in between makes it explicit. Incidentally, the comment
> > satisfies Coverity as well.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > index 6594b8eae83f..eda97285ee18 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > @@ -1428,6 +1428,9 @@ UpdatePciInfo (
> >                switch (Ptr->AddrSpaceGranularity) {
> >                  case 32:
> >                    PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypeMem32;
> > +                  //
> > +                  // No break; here as this is an intentional fall
> through.
> > +                  //
> >                  case 64:
> >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> >                    break;
> > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> >                switch (Ptr->AddrSpaceGranularity) {
> >                  case 32:
> >                    PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypePMem32;
> > +                  //
> > +                  // No break; here as this is an intentional fall
> through.
> > +                  //
> >                  case 64:
> >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> >                    break;
>
> Agree, but the semicolon's placement is awkward. I propose
>
>   No break here, as this is an intentional fall through.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
>
>
>
> 
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110892): https://edk2.groups.io/g/devel/message/110892
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 11149 bytes --]

  reply	other threads:[~2023-11-08  4:05 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-07  6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue Ranbir Singh
2023-11-07 16:21   ` Laszlo Ersek
2023-11-10  6:14     ` Ranbir Singh
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh
2023-11-07  8:15   ` Ni, Ray
2023-11-07 16:23   ` Laszlo Ersek
2023-11-07 17:59     ` Michael D Kinney
2023-11-08  3:51       ` Ranbir Singh
2023-11-08  4:05         ` Michael D Kinney [this message]
2023-11-08  4:29           ` Ranbir Singh
2023-11-13 11:24             ` Laszlo Ersek
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON " Ranbir Singh
2023-11-07 16:41   ` Laszlo Ersek
2023-11-10  5:59     ` Ranbir Singh
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue Ranbir Singh
2023-11-07 16:48   ` Laszlo Ersek
2023-11-10  6:11     ` Ranbir Singh
2023-11-13 11:28       ` Laszlo Ersek
2023-11-14 15:08         ` Ranbir Singh
2023-11-14 16:21           ` Michael D Kinney
2023-11-14 16:53             ` Ranbir Singh
2023-11-15 10:02               ` Laszlo Ersek
2023-11-14 19:37             ` Michael Kubacki
2023-11-15 10:10               ` Laszlo Ersek
2023-11-20 14:05                 ` Michael Kubacki
2023-11-15  9:50             ` Laszlo Ersek
2023-11-20  3:57               ` Ranbir Singh
2023-11-21 17:07                 ` Laszlo Ersek
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues Ranbir Singh
2023-11-07 17:20   ` Laszlo Ersek
2023-11-10  6:31     ` Ranbir Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=SA2PR11MB49388C38F197FBBAB538594BD2A8A@SA2PR11MB4938.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox