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

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.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: Mon, 25 Apr 2022 13:00:57 +0300	[thread overview]
Message-ID: <20220425100057.GB2090@kadam> (raw)
In-Reply-To: <YmZp+qgSLpT5PP2u@aptenodytes>

On Mon, Apr 25, 2022 at 11:29:30AM +0200, Paul Kocialkowski wrote:
> > 
> > No.  You are just making the code ugly and complicated for no reason.
> > 
> > I work in static analysis so I have focussed a lot of attention on
> > locking bugs.  In real life this theory is totally bogus.  Single exit
> > paths only cause bugs, they don't prevent bugs.
> 
> I'm really surprised by this and honestly it feels a bit dogmatic.
> 
> It reminds me of CS teachers telling me "gotos are evil and you must
> never use them". In practice there are many situations where they make
> the code more readable and don't introduce any significant incertainty.

Gotos are fine.  Backwards gotos are horrible, but sometimes necessary.
But pointless gotos are bad.  And "out" is a bad label name.

	return -ENOMEM;

That's perfectly readable.

	ret = -ENOMEM;
	goto out;

That's vague.  The out label is likely to do nothing or everything.  The
problem with do-nothing gotos are that people forget to set the error
code.  Also it interrupts the reader, now you have to scroll to the
bottom of the function, you have lost your train of thought, and then
you have scroll back and find your place again.

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.

Another problem with do-everything error handling is that eventually it
gets too complicated so people just leave the error handling out.  It's
hard to audit to see if everything is freed.

With static analysis, I'm mostly looking at error handling instead of
success paths paths.  The one error label style is the by far the
worst.

People think single labels will prevent locking bugs.  It doesn't work.
There is two people who use indenting to remind them about locks:

	lock(); {
		frob();
		frob();
		frob();
	} unlock();

And occasionally they still forget to drop the lock before returning on
error paths.  Nothing works for forgot to drop the lock bugs except
static analysis.

regards,
dan carpenter

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