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

[Paul Kocialkowski]

Hi Dan,

On Mon 25 Apr 22, 13:00, Dan Carpenter wrote:
> 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.

Okay I think these are good points, fair enough.

> 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?

I've also seen conditionals used in error labels to undo stuff.

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.

Thanks for the explanation!

Paul

> 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

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20220426/a34c1c1c/attachment.sig>