From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D8CA1C47DD9 for ; Wed, 28 Feb 2024 11:09:43 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.686473.1068565 (Exim 4.92) (envelope-from ) id 1rfHoR-0006s9-4C; Wed, 28 Feb 2024 11:09:23 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 686473.1068565; Wed, 28 Feb 2024 11:09:23 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1rfHoR-0006s2-1R; Wed, 28 Feb 2024 11:09:23 +0000 Received: by outflank-mailman (input) for mailman id 686473; Wed, 28 Feb 2024 11:09:21 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1rfHoP-0006rw-22 for xen-devel@lists.xenproject.org; Wed, 28 Feb 2024 11:09:21 +0000 Received: from support.bugseng.com (mail.bugseng.com [162.55.131.47]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id d1bba512-d629-11ee-afd7-a90da7624cb6; Wed, 28 Feb 2024 12:09:19 +0100 (CET) Received: from support.bugseng.com (support.bugseng.com [162.55.131.47]) by support.bugseng.com (Postfix) with ESMTPA id 733944EE0737; Wed, 28 Feb 2024 12:09:19 +0100 (CET) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: d1bba512-d629-11ee-afd7-a90da7624cb6 MIME-Version: 1.0 Date: Wed, 28 Feb 2024 12:09:19 +0100 From: Nicola Vetrini To: Stefano Stabellini Cc: Jan Beulich , Julien Grall , consulting@bugseng.com, Andrew Cooper , George Dunlap , Wei Liu , xen-devel@lists.xenproject.org Subject: Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7 In-Reply-To: References: <33342a17-e71c-4752-a16f-da5c0ef77b51@suse.com> <2178731a-ec81-4505-ba8a-2f945bf85133@suse.com> <7a8e610e-913e-4a56-8ce1-6dd6abd894f4@xen.org> <4bee79ca-7a7e-4bcc-ac97-5a5a57ec2c91@suse.com> Message-ID: <6af04933659178b3ccabc5caf646273c@bugseng.com> X-Sender: nicola.vetrini@bugseng.com Organization: BUGSENG s.r.l. Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit On 2024-02-28 03:10, Stefano Stabellini wrote: > On Tue, 27 Feb 2024, Jan Beulich wrote: >> On 27.02.2024 12:52, Julien Grall wrote: >> > Hi Jan, >> > >> > On 27/02/2024 07:28, Jan Beulich wrote: >> >> On 27.02.2024 01:26, Stefano Stabellini wrote: >> >>> On Mon, 26 Feb 2024, Jan Beulich wrote: >> >>>> On 23.02.2024 10:35, Nicola Vetrini wrote: >> >>>>> Refactor cpu_notifier_call_chain into two functions: >> >>>>> - the variant that is allowed to fail loses the nofail flag >> >>>>> - the variant that shouldn't fail is encapsulated in a call >> >>>>> to the failing variant, with an additional check. >> >>>>> >> >>>>> This prevents uses of the function that are not supposed to >> >>>>> fail from ignoring the return value, thus violating Rule 17.7: >> >>>>> "The value returned by a function having non-void return type shall >> >>>>> be used". >> >>>>> >> >>>>> No functional change. >> >>>>> >> >>>>> Signed-off-by: Nicola Vetrini >> >>>> >> >>>> I'm afraid I disagree with this kind of bifurcation. No matter what >> >>>> Misra thinks or says, it is normal for return values of functions to >> >>>> not always be relevant to check. >> >>> >> >>> Hi Jan, I disagree. >> >>> >> >>> Regardless of MISRA, I really think return values need to be checked. >> >>> Moreover, we decided as a group to honor MISRA Rule 17.7, which requires >> >>> return values to be checked. This patch is a good step forward. >> >> >> >> Yet splitting functions isn't the only way to deal with Misra's >> >> requirements, I suppose. After all there are functions where the >> >> return value is purely courtesy for perhaps just one of its callers. >> > >> > You are right that we have some places where one caller care about the >> > return value. But the problem is how do you tell whether the return was >> > ignored on purpose or not? >> > >> > We had at least one XSA because the return value of a function was not >> > checked (see XSA-222). We also had plenty of smaller patches to check >> > returns. >> > >> > So far, we added __must_check when we believed return values should be >> > checked. But usually at the point we notice, this is far too late. >> > >> > To me the goal should be that we enforce __must_check everywhere. We are >> > probably going to detect places where we forgot to check the return. For >> > thoses that are on purpose, we can document them. >> > >> >> >> >> Splitting simply doesn't scale very well, imo. >> > >> > Do you have another proposal? As Stefano said, we adopted the rule 17.7. >> > So we know need a solution to address it. >> >> One possibility that was circulated while discussing was to add (void) >> casts. I'm not a huge fan of those, but between the two options that >> might be the lesser evil. We also use funny (should I say ugly) >> workarounds in a few cases where we have __must_check but still want >> to not really handle the return value in certain cases. Given there >> are >> example in the code base, extending use of such constructs is >> certainly >> also something that may want considering. > > I asked Roberto if void casts are an option for compliance. > void casts are an option for sure. The rationale for the rule explicitly lists them as a compliance mechanism. An interesting aspect is what would be the consensus around void casts on functions whose return value is always ignored vs. functions whose return value is sometimes ignored. > In any case, I don't think we should use void casts in the specific > cases this patch is dealing with. Void casts (if anything) should be a > last resort while this patch fixes the issue in a better way. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)