Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Ian Cowan <ian@linux.cowan.aero>
Cc: mripard@kernel.org, paul.kocialkowski@bootlin.com,
	mchehab@kernel.org, gregkh@linuxfoundation.org, wens@csie.org,
	jernej.skrabec@gmail.com, samuel@sholland.org,
	linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev
Subject: Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit
Date: Mon, 25 Apr 2022 12:36:36 +0300	[thread overview]
Message-ID: <20220425093636.GM2462@kadam> (raw)
In-Reply-To: <20220423180111.91602-1-ian@linux.cowan.aero>

On Sat, Apr 23, 2022 at 02:01:11PM -0400, Ian Cowan wrote:
> Refactor the cedrus_open() function so that there is only one exit to
> the function instead of 2. This prevents a future change from preventing
> the mutex from being unlocked after a successful exit.

Future proofing code is a complicated thing because the future is hard
to predict.

But one rule is that normally the success path gets tested.  No one is
going to forget to drop the lock on the success path.  Or if we do forget,
it likely will get caught quickly and it will be easy to bisect.

Generally it's best to keep the success path and the failure path as
separate as possible.  Avoid interleaving.  It's more readable.  If
there is no cleanup on failure then it's okay to have:

unlock:
	unlock(foo);
	return ret;

But once you have cleanup then put that in a separate cleanup block.

As well Smatch can recognize a typical kernel cleanup block so if you
write it in a weird way then you're disabling the static checker warning
for missing error codes.  I probably am going to write more checks which
are specific to cleanup blocks in the future.

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel