From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 8E5547803D7 for ; Tue, 21 Nov 2023 17:07:16 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=yJ1mAfLNW5h/e30x7+hD60IkNuwobKqFuZiO2GtNQ44=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700586435; v=1; b=c5lo7+Py85MiQSCRm97DtLYCdpEGlyerPf5RnFFtCnjKb3ustGc7RGYv2V8bKhMDiMgN37b8 sxgnswZYd/yucS3qzyY6fscjtW4m2EKxvp67r8mf/eMY0XXdAXjZ1i8oazaSNdnmJByIEYy8ybL 2hLuPUcE8zOkGeG5f7roRqAc= X-Received: by 127.0.0.2 with SMTP id vufvYY7687511xtgUT7Ve0PM; Tue, 21 Nov 2023 09:07:15 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.47390.1700586434598717664 for ; Tue, 21 Nov 2023 09:07:14 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-500-Yw6FZnvEMx2aOTDIAe8fgw-1; Tue, 21 Nov 2023 12:07:12 -0500 X-MC-Unique: Yw6FZnvEMx2aOTDIAe8fgw-1 X-Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 94FAB1C05192; Tue, 21 Nov 2023 17:07:11 +0000 (UTC) X-Received: from [10.39.194.228] (unknown [10.39.194.228]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0BAD2492BE0; Tue, 21 Nov 2023 17:07:09 +0000 (UTC) Message-ID: <43e829d3-d755-1c55-5202-995b3d6f0987@redhat.com> Date: Tue, 21 Nov 2023 18:07:08 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue To: devel@edk2.groups.io, rsingh@ventanamicro.com Cc: "Kinney, Michael D" , "Andrew Fish (afish@apple.com)" , "quic_llindhol@quicinc.com" , Michael Kubacki , "Ni, Ray" , Veeresh Sangolli References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-5-rsingh@ventanamicro.com> <5a7f3c56-d18f-9e4c-0e70-9113923ee36d@redhat.com> <5443d245-0631-a33f-220f-81e39e33af4a@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 6eNgIJQPmTsqJzkEMeStLLTPx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=c5lo7+Py; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 11/20/23 04:57, Ranbir Singh wrote: > > > On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek > 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 > > > > > > 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] -=-=-=-=-=-=-=-=-=-=-=-