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

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

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.

> 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 (#111247): https://edk2.groups.io/g/devel/message/111247
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]
-=-=-=-=-=-=-=-=-=-=-=-