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 3C5BED800C4 for ; Wed, 15 Nov 2023 09:50:39 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=s6FUGbS6tNVPPPRrxzZM/evF7V4peyK5+l2U6YSJEHs=; 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=1700041837; v=1; b=LAI1Y7UcYVZnhnFPaWgVz2aTQ6kSWmytpIT8v4RL+lYTbq3CgYET/7PXlSLMnHN0O2JRFlZF NXV/13nO5kK5pmeeAmKPvbHnT+xQ5GJLkzHcA1CxdDCl0MXQZ8266WjWJ1KJyDrXYDOp2POiQ7R /MOKqZXLGzzBidAhACJMtrMg= X-Received: by 127.0.0.2 with SMTP id UO4AYY7687511xZKp2ZrptJY; Wed, 15 Nov 2023 01:50:37 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.9863.1700041837202262764 for ; Wed, 15 Nov 2023 01:50:37 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-625-gpoNXvYtPqeN2bIaP6v-fg-1; Wed, 15 Nov 2023 04:50:31 -0500 X-MC-Unique: gpoNXvYtPqeN2bIaP6v-fg-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (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 A4FFE810FC0; Wed, 15 Nov 2023 09:50:30 +0000 (UTC) X-Received: from [10.39.192.211] (unknown [10.39.192.211]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4B9F9C1596F; Wed, 15 Nov 2023 09:50:29 +0000 (UTC) Message-ID: <5443d245-0631-a33f-220f-81e39e33af4a@redhat.com> Date: Wed, 15 Nov 2023 10:50:27 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue To: "Kinney, Michael D" , "devel@edk2.groups.io" , "rsingh@ventanamicro.com" , "Andrew Fish (afish@apple.com)" , "quic_llindhol@quicinc.com" , Michael Kubacki Cc: "Ni, Ray" , Veeresh Sangolli References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-5-rsingh@ventanamicro.com> <5a7f3c56-d18f-9e4c-0e70-9113923ee36d@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 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: 1tniHAQovOIjb8s7gfwkIANrx7686176AA= 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=LAI1Y7Uc; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) 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. > 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] -=-=-=-=-=-=-=-=-=-=-=-