Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

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

On 11/20/23 04:57, Ranbir Singh wrote:
> 
> 
> On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 11/14/23 17:21, Kinney, Michael D wrote:
>     > Hi Ranbir,
>     >
>     >  
>     >
>     > First I want to recognize your efforts to collect Coverity issues and
>     > propose changes to address
>     > them.
>     >
>     > I still disagree with adding CpuDealLoop() for any static analysis
>     issues.
>     >
>     > There have been previous discussions about adding a PANIC() or FATAL()
>     > macro that would
>     > perform platform specific actions if a condition is detected where the
>     > boot of the platform
>     > can not continue.  A platform get to make the choice to log or
>     reboot or
>     > hang, etc.  Not the
>     > code that detected the condition.
>     >
>     > https://edk2.groups.io/g/devel/message/86597
>     <https://edk2.groups.io/g/devel/message/86597>
>     > <https://edk2.groups.io/g/devel/message/86597
>     <https://edk2.groups.io/g/devel/message/86597>>
> 
>     This is indeed a great idea.
> 
>     I didn't know about that discussion. Perhaps a new DebugLib API would
>     handle this (i.e., "panic").
> 
>     I've been certainly proposing CpuDeadLoop() as a means to panic.
> 
>     Did the thread conclude anything tangible?
> 
>     > Unfortunately, in order to fix some of these static analysis issues
>     > correctly, we are going
>     > to have to identify the use of ASSERT() that really is a fatal
>     condition
>     > that can not continue
> 
>     Absolutely.
> 
>     > and introduce an implementation approach that provides a platform
>     > handler and
>     > satisfies the static analysis tools.
> 
>     The "platform handler" is the difficult part. If the above-noted
>     discussion from 2022 didn't produce an agreement, should we really block
>     the static analyzer fixes on an agreed-upon panic API? I'm concerned
>     that would just cause these fixes to get stuck. And I don't consider
>     CpuDeadLoop()s added for this purpose serious technical debt. They are
>     easy to find and update later, assuming we also add comments.
> 
> 
> I agree with the approach to not gate current fixes adding
> CpuDeadLoop(). Later on, it can be updated with the desired panic API
> and I can contribute for those required changes related to patches
> submitted by me.
> 
> I can update current patches to carry additional comment in suffix form
> to ease later search like
>     CpuDeadLoop (); // TBD: replace with Panic API in future
> 
> Laszlo, Mike - Let me know if that works for now.

It works for me.

Of course the risk is always that the proper panic API might never
materialize, and then we'll be stuck with these comments forever as yet
another piece of technical debt. From that perspective minimally, it
would be reasonable for Mike not to accept these reminder comments. (A
reminder BZ, with the commit hash and exact code locations listed, could
be a stronger reminder, but we've seen such BZs too fall through the
cracks over time.)

So, for me, I'm OK; but if Mike doesn't like this approach, I'll
certainly accept that (and then we can't fix the coverity warnings until
the panic API arrives).

Thanks
Laszlo

>  
> 
>     > We also have to evaluate if a return error status with a DEBUG_ERROR
>     > message would be a better
>     > choice than an ASSERT() that can be filtered out by build options.
> 
>     I agree 100% that this would be better for the codebase, but the work
>     needed for this is (IMO) impossible to contain. ASSERT() has been abused
>     for a long time *because* it seemed to allow the programmer to ignore
>     any related error paths. If we now want to implement those error paths
>     retroactively (which would be absolutely the right thing to do from a
>     software engineering perspective), then immense amounts of work are
>     going to be needed (patch review and regression testing), and I think
>     it's just not practical to dump all that on someone that wants to
>     improve the status quo. Replacing an invalid ASSERT() with a panic is
>     honest about the current situation, is safer regarding RELEASE builds,
>     and its work demand (regression testing, patch review) is tolerable.
> 
>     I do agree that, if the error path mostly exists already, then returning
>     errors for data/environment-based error conditions (i.e., not for
>     algorithmic invariant failures) is best.
> 
>     Where we need to be extremely vigilant is new patches. We must
>     uncompromisingly reject *new* abuses of ASSERT(), in new patches.
> 
>     Anyway, it seems that we've been trying to steer Ranbir in opposite
>     directions. I'll let you take the lead on this; for one, I've not been
>     aware of the panic api discussion for 2022!
> 
>     (I don't feel especially pushy about fixing coverity issues, it's just
>     that Ranbir has been contributing such patches, which I've found very
>     welcome, and I wanted to help out with reviews.)
> 
>     Laszlo
> 
> 



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