staging: sunxi: cedrus: centralize cedrus_open exit

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.

Signed-off-by: Ian Cowan <ian@linux.cowan.aero>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Hi Dan,

On Mon 25 Apr 22, 12:20, Dan Carpenter wrote:
> 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.
> > 
> > Signed-off-by: Ian Cowan <ian@linux.cowan.aero>
> 
> 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.

In this instance I don't see what could possible go wrong and I agree it
makes the code more readable.

Cheers,

Paul

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

Hi Ian,

Dne sobota, 23. april 2022 ob 20:01:11 CEST je Ian Cowan napisal(a):
> 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.
> 
> Signed-off-by: Ian Cowan <ian@linux.cowan.aero>

If this patch would be part of series and "future" would mean next patch, I 
would be ok with that. However, in current form I don't see any benefit in 
changing it. Doing another hop lessens readability IMO. Let us worry about the 
future when/if it comes.

Best regards,
Jernej

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/
media/sunxi/cedrus/cedrus.c
> index 68b3dcdb5df3..5236d9e4f4e8 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -348,14 +348,14 @@ static int cedrus_open(struct file *file)
>  
>  	v4l2_fh_add(&ctx->fh);
>  
> -	mutex_unlock(&dev->dev_mutex);
> -
> -	return 0;
> +	ret = 0;
> +	goto succ_unlock;
>  
>  err_ctrls:
>  	v4l2_ctrl_handler_free(&ctx->hdl);
>  err_free:
>  	kfree(ctx);
> +succ_unlock:
>  	mutex_unlock(&dev->dev_mutex);
>  
>  	return ret;
> -- 
> 2.35.1
> 
>

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