[edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

Hi Mike,

I earlier assumed that both the fallthrough were intentional and just made
it evident by writing the comment. It was hence written actually for the
code reader and not the static analyzer. The comment was similar in nature
and style to existing No/1-byte/... data comments. *Incidentally, the
comment satisfied the static analyzer as well. *It was beyond my
expectation and I was pleasantly surprised too. Without digging deep into
the particular code and to avoid any functional code change, I just thought
that's it.

Your comment forced me to come back specifically to the particular code and
to analyze it a bit more.

My analysis now says that the fallthrough doesn't serve any real functional
purpose as there is absolutely no chance of *if* blocks of the fallthrough
*case(*s) being executed later and not executed in the original *case:* and
hence the fallthrough if it happens still leads to execution of *return
NULL; *at the end of the function only. This is better communicated by
adding *break;* statement in the original *case:* and in a way, one may
agree with the static analyzer that there was a MISSING_BREAK. I should
hence follow it up with a v2 patch for this.

Ranbir Singh

On Thu, Oct 5, 2023 at 12:17 AM Kinney, Michael D <
michael.d.kinney at intel.com> wrote:

> I do not prefer special comments for one static analyzer.
>
> Is there an alternative design/implementation of this code
> to make it more readable and not trigger any static analysis
> false positives?
>
> Mike
>
> > -----Original Message-----
> > From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ranbir
> > Singh
> > Sent: Tuesday, October 3, 2023 10:48 PM
> > To: devel at edk2.groups.io; rsingh at ventanamicro.com
> > Cc: Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>;
> > Veeresh Sangolli <veeresh.sangolli at dellteam.com>
> > Subject: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe:
> > Fix MISSING_BREAK Coverity issues
> >
> > From: Ranbir Singh <Ranbir.Singh3 at Dell.com>
> >
> > The function GetNextHidItem has a switch-case code in which the
> > case 1: falls through to case 2: and then case 2: falls through
> > to case 3: in the block.
> >
> > While this may be intentional, it is not evident to any general
> > code reader as well as any static analyzer tool. Just adding
> >
> >     // No break; here as this is an intentional fallthrough.
> >
> > as comment in between makes a reader as well as Coverity happy.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
> >
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli at dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3 at Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh at ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > index acc19acd98e0..bc9a4824208b 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > @@ -89,6 +89,10 @@ GetNextHidItem (
> >            return StartPos;
> >
> >          }
> >
> >
> >
> > +        //
> >
> > +        // No break; here as this is an intentional fallthrough
> >
> > +        //
> >
> > +
> >
> >        case 2:
> >
> >          //
> >
> >          // 2-byte data
> >
> > @@ -99,6 +103,10 @@ GetNextHidItem (
> >            return StartPos;
> >
> >          }
> >
> >
> >
> > +        //
> >
> > +        // No break; here as this is an intentional fallthrough
> >
> > +        //
> >
> > +
> >
> >        case 3:
> >
> >          //
> >
> >          // 4-byte data, adjust size
> >
> > --
> > 2.34.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#109309):
> > https://edk2.groups.io/g/devel/message/109309
> > Mute This Topic: https://groups.io/mt/101750274/1643496
> > Group Owner: devel+owner at edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kinney at intel.com]
> > -=-=-=-=-=-=
> >
>
>


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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20231005/492698f7/attachment.htm>