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

From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Ian Cowan <ian@linux.cowan.aero>,
	mripard@kernel.org, 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: Thu, 28 Apr 2022 13:56:49 +0200	[thread overview]
Message-ID: <YmqBAZwdiE6GuarU@aptenodytes> (raw)
In-Reply-To: <20220428102608.GU2462@kadam>

[-- Attachment #1.1: Type: text/plain, Size: 1715 bytes --]

Hi Dan,

On Thu 28 Apr 22, 13:26, Dan Carpenter wrote:
> On Tue, Apr 26, 2022 at 09:39:03AM +0200, Paul Kocialkowski wrote:
> > 
> > > Do-everything gotos are the most bug prone style of error handling.
> > > Imagine the function is trying to do three things.  It fails part way
> > > through.  Now you're trying to undo the second thing which was never
> > > done.  Just moments ago I was just looking at one of these do-everything
> > > bugs where it was using uninitialized memory.
> > 
> > So by that you mean having just one label for all error handling instead
> > of labels for each undo step?
> > 
> 
> Yes.  Don't do that.  If you try to free everything, half the stuff is
> not allocated so you will undo things which have not been done and it
> leads to a bug.
>
> > I've also seen conditionals used in error labels to undo stuff.
> > 
> 
> I don't understand what you're describing?

Typically that would look like:

void *foo = NULL;
void *bar = NULL;

foo = alloc(...);
if (!foo)
	goto single_error;

bar = alloc(...);
if (!bar)
	goto single_error;

...

single_error:
	if (bar)
		free(bar);

	if (foo)
		free(foo);

> > Would you recommend duplicating error cleanup in each error condition?
> > It feels like another set of issue on its own, besides the obvious downside
> > of duplication.
> 
> Let me write a blog about it:
> 
> https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

Good writeup, thanks! The part about unwinding loops especially, I've always
wondered about the right way to go about it.

Cheers,

Paul 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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