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

["Ranbir Singh" <rsingh@ventanamicro.com>]

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

On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek <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>
>
> 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.


> > 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 (#111444): https://edk2.groups.io/g/devel/message/111444
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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