Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

From: Markus Elfring <Markus.Elfring@web.de>
To: Nathan Lynch <nathanl@linux.ibm.com>,
	linuxppc-dev@lists.ozlabs.org, kernel-janitors@vger.kernel.org
Cc: Paul Moore <paul@paul-moore.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Nicholas Piggin <npiggin@gmail.com>,
	cocci@inria.fr
Subject: Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()
Date: Sat, 18 Mar 2023 08:30:24 +0100	[thread overview]
Message-ID: <2f5a00f6-f3fb-9f00-676a-acdcbef90c6c@web.de> (raw)
In-Reply-To: <87v8iz75ck.fsf@linux.ibm.com>

>>>> The label “out_err” was used to jump to another pointer check despite of
>>>> the detail in the implementation of the function “pSeries_reconfig_add_node”
>>>> that it was determined already that the corresponding variable contained
>>>> a null pointer (because of a failed function call in two cases).
>>>>
>>>> 1. Thus return directly after a call of the function “kzalloc” failed.
>>>>
>>>> 2. Use more appropriate labels instead.
>>>>
>>>> 3. Delete a redundant check.
>>>>
>>>> 4. Omit an explicit initialisation for the local variable “err”.
>>>>
>>>> This issue was detected by using the Coccinelle software.
>>> Is there a correctness or safety issue here?
>> I got the impression that the application of only a single label like “out_err”
>> resulted in improvable implementation details.
> I don't understand what you're trying to say here.

What does hinder you to understand the presented change description better
at the moment?

> It doesn't seem to answer my question.

I hope that my answer will trigger further helpful considerations.

>>> The subject uses the word "fix" but the commit message doesn't seem to identify one.
>> Can you find the proposed adjustments reasonable?
> In the absence of a bug fix or an improvement in readability, not really, sorry.

The views are varying for “programming bugs”, aren't they?

> It adds to the function more goto labels and another return,

This is the suggested source code transformation.

> apparently to avoid checks

Can the support grow for such a programming goal?

> that are sometimes redundant

Can such implementation details become undesirable?

> (but not incorrect) at the C source code level.

Will this aspect affect further development concerns?

>> Please take another look at available information sources.
>> https://lore.kernel.org/cocci/f9303bdc-b1a7-be5e-56c6-dfa8232b8b55@web.de/
> I wasn't cc'd on this and I'm not subscribed to any lists in the recipients
> for that message, so not sure how I would take "another" look. :-)

I imagine that you can benefit more from information which can be retrieved
by archive interfaces also according to the mailing list of the Coccinelle software.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/coccinelle.rst?h=v6.3-rc2#n9

Regards,
Markus