[PATCH] staging: unisys: use common return path

[PATCH] staging: unisys: use common return path

[Sudip Mukherjee]

The success path and the error path both are first doing
spin_unlock_irqrestore() before returning. Use that in the common path
and return the success/error value.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

It is dependent on the patch series sent by Benjamin (Nov 30th).

 drivers/staging/unisys/visornic/visornic_main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 77fd1ef..342c0bf 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -357,15 +357,16 @@ visornic_serverdown(struct visornic_devdata *devdata,
 		    visorbus_state_complete_func complete_func)
 {
 	unsigned long flags;
+	int retval = 0;
 
 	spin_lock_irqsave(&devdata->priv_lock, flags);
 	if (!devdata->server_down && !devdata->server_change_state) {
 		if (devdata->going_away) {
-			spin_unlock_irqrestore(&devdata->priv_lock, flags);
 			dev_dbg(&devdata->dev->device,
 				"%s aborting because device removal pending\n",
 				__func__);
-			return -ENODEV;
+			retval = -ENODEV;
+			goto error;
 		}
 		devdata->server_change_state = true;
 		devdata->server_down_complete_func = complete_func;
@@ -374,11 +375,12 @@ visornic_serverdown(struct visornic_devdata *devdata,
 	} else if (devdata->server_change_state) {
 		dev_dbg(&devdata->dev->device, "%s changing state\n",
 			__func__);
-		spin_unlock_irqrestore(&devdata->priv_lock, flags);
-		return -EINVAL;
+		retval = -EINVAL;
 	}
+
+error:
 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
-	return 0;
+	return retval;
 }
 
 /**
-- 
1.9.1

Re: [PATCH] staging: unisys: use common return path

[Dan Carpenter]

Doing One Err style error handling is often a mistake but it's ok here.
Just choose a better label name, though.  "unlock".  I have seen several
bugs caused because people used the label name "err" instead of
"unlock".  Compare these two examples.

	spin_unlock();
err:
	return ret;

Do you see the bug?  Now look at this code:

	spin_unlock();
unlock:
	return ret;

Just from looking at those few lines you can see that the intent was
to unlock but instead it just returns.  In the first example, you need
to scroll to the start of the function and examine the context to see
what "err" was intended to do.  And you're not likely to even be
suspicious of the err label because do nothing labels with ambiguos
label names are very common.

I have seen at least three places where an ambiguously named label was
placed after the spin unlock instead of before, where it was intended.

regards,
dan carpenter

Re: [PATCH] staging: unisys: use common return path

[Dan Carpenter]

Also, the main point of course is that you should choose meaningful
names for labels every time just like you would for functions.

regards,
dan carpenter

Re: [PATCH] staging: unisys: use common return path

[Sudip Mukherjee]

On Tue, Dec 01, 2015 at 11:06:58AM +0300, Dan Carpenter wrote:
> Also, the main point of course is that you should choose meaningful
> names for labels every time just like you would for functions.

Yes, and while sending I was really thinking that you are going to
complain about the label name. :)
But could not think of a better name. I think I need to take a lesson
from you about the namings.

regards
sudip

Re: [PATCH] staging: unisys: use common return path

[Dan Carpenter]

A lot of review is judgement calls.  I try very hard to be predictable
and machine like as possible.

I have sometimes called you out for being too strict.  I worry sometimes
that I am too strict.  I ignore a lot of stuff.  Feel free to call me
out though if you want because I welcome feedback.

regards,
dan carpenter

Re: [PATCH] staging: unisys: use common return path

[Sudip Mukherjee]

On Tue, Dec 01, 2015 at 07:05:19PM +0300, Dan Carpenter wrote:
> A lot of review is judgement calls.  I try very hard to be predictable
> and machine like as possible.
> 
> I have sometimes called you out for being too strict.

I usually try to guess what Greg will do if he notices those multiple
changes (for which you called me strict). But since staging is meant to
be a learning ground for newbies, so my being strict will, most of the
time, be for for them.

regards
sudip

Re: [PATCH] staging: unisys: use common return path

[Ben Romer]

On 12/01/2015 03:00 AM, Dan Carpenter wrote:
> Doing One Err style error handling is often a mistake but it's ok here.

Why is it okay here? I don't understand why this function would be any 
different than the other places where the code used a goto.

If we *have* to change it I would prefer that we not add a goto and 
instead add an additional boolean local variable to control serverdown 
completion. That's less complex and makes the intent clear.

like this:

visornic_serverdown(struct visornic_devdata *devdata,
		    visorbus_state_complete_func complete_func)
{
	unsigned long flags;
	int retval = 0;
	bool complete_serverdown = false;

	spin_lock_irqsave(&devdata->priv_lock, flags);
	if (!devdata->server_down && !devdata->server_change_state) {
		if (devdata->going_away) {
			dev_dbg(&devdata->dev->device,
				"%s aborting because device removal pending\n",
				__func__);
			retval = -ENODEV;
		} else {
			devdata->server_change_state = true;
			devdata->server_down_complete_func = complete_func;
			complete_serverdown = true;
		}
	} else if (devdata->server_change_state) {
		dev_dbg(&devdata->dev->device, "%s changing state\n",
			__func__);
		spin_unlock_irqrestore(&devdata->priv_lock, flags);
		retval = -EINVAL;
	} 		
	spin_unlock_irqrestore(&devdata->priv_lock, flags);
	
	if (complete_serverdown)
		visornic_serverdown_complete(devdata);

	return retval;
}

-- Ben

Re: [PATCH] staging: unisys: use common return path

[Dan Carpenter]

On Tue, Dec 01, 2015 at 09:54:50AM -0500, Ben Romer wrote:
> On 12/01/2015 03:00 AM, Dan Carpenter wrote:
> >Doing One Err style error handling is often a mistake but it's ok here.
> 
> Why is it okay here? I don't understand why this function would be
> any different than the other places where the code used a goto.

What I meant was that I'm generally opposed to "common exit paths".
Mixing all the exit paths together often makes the code more complicated
and leads to errors.  That makes sense from a common sense perspective
that doing many things is more difficult than doing one thing?  Anyway
it's easy enough to verify empirically that this style is bug prone.

On the other hand there are times where all exit paths need to unlock or
to free a variable and in those cases using a common exit path makes
sense.  Just don't standardize on "Every function should only have a
single return".

> 
> If we *have* to change it

I don't think we have to change it at all.  Using direct returns makes
finding locking bugs easier for static checkers.

> I would prefer that we not add a goto and
> instead add an additional boolean local variable to control
> serverdown completion. That's less complex and makes the intent
> clear.
> 

I don't really like this but I also don't care because the function is
short.  The reason I don't like is because it force you to scroll back
up and ideally you could understand a function by reading it from top to
bottom without scrolling backwards.

> like this:
> 
> visornic_serverdown(struct visornic_devdata *devdata,
> 		    visorbus_state_complete_func complete_func)
> {
> 	unsigned long flags;
> 	int retval = 0;
> 	bool complete_serverdown = false;
> 
> 	spin_lock_irqsave(&devdata->priv_lock, flags);
> 	if (!devdata->server_down && !devdata->server_change_state) {
> 		if (devdata->going_away) {
> 			dev_dbg(&devdata->dev->device,
> 				"%s aborting because device removal pending\n",
> 				__func__);
> 			retval = -ENODEV;
> 		} else {
> 			devdata->server_change_state = true;
> 			devdata->server_down_complete_func = complete_func;
> 			complete_serverdown = true;
> 		}
> 	} else if (devdata->server_change_state) {
> 		dev_dbg(&devdata->dev->device, "%s changing state\n",
> 			__func__);
> 		spin_unlock_irqrestore(&devdata->priv_lock, flags);

This is a bug.

> 		retval = -EINVAL;
> 	} 		
> 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
> 	
> 	if (complete_serverdown)
> 		visornic_serverdown_complete(devdata);


regards,
dan carpenter

Re: [PATCH] staging: unisys: use common return path

[Ben Romer]

On 12/01/2015 10:57 AM, Dan Carpenter wrote:
> What I meant was that I'm generally opposed to "common exit paths".
> Mixing all the exit paths together often makes the code more complicated
> and leads to errors.  That makes sense from a common sense perspective
> that doing many things is more difficult than doing one thing?  Anyway
> it's easy enough to verify empirically that this style is bug prone.
>
> On the other hand there are times where all exit paths need to unlock or
> to free a variable and in those cases using a common exit path makes
> sense.  Just don't standardize on "Every function should only have a
> single return".
>

That works for me. Mainly my issue with it is that I've spent a lot of 
time trying to eliminate "goto Away" code from the drivers, so I'd 
rather not put any back if possible.

>>
>> If we *have* to change it
>
> I don't think we have to change it at all.  Using direct returns makes
> finding locking bugs easier for static checkers.
>

That's true, and I think the code is fine as it is.

>> 		spin_unlock_irqrestore(&devdata->priv_lock, flags);
>
> This is a bug.
>

Indeed, but I'd rather not have any of these changes made anyway. This 
function isn't broken so it doesn't need to be fixed.

-- Ben

Re: [PATCH] staging: unisys: use common return path

[Sudip Mukherjee]

On Tue, Dec 01, 2015 at 11:16:16AM -0500, Ben Romer wrote:
> On 12/01/2015 10:57 AM, Dan Carpenter wrote:
> >What I meant was that I'm generally opposed to "common exit paths".
> >Mixing all the exit paths together often makes the code more complicated
> >and leads to errors.  That makes sense from a common sense perspective
> >that doing many things is more difficult than doing one thing?  Anyway
> >it's easy enough to verify empirically that this style is bug prone.
> >
> >On the other hand there are times where all exit paths need to unlock or
> >to free a variable and in those cases using a common exit path makes
> >sense.  Just don't standardize on "Every function should only have a
> >single return".
> >
> 
> That works for me. Mainly my issue with it is that I've spent a lot
> of time trying to eliminate "goto Away" code from the drivers, so
> I'd rather not put any back if possible.

But what is wrong with goto?

Quoting from CodingStyle:
"The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.  If there
is no cleanup needed then just return directly."

I am absolutely fine if you don't want it to be applied but just for
knowing - 
It has multiple exits.
In this case spin_unlock_irqrestore() is the common work.

regards
sudip

Re: [PATCH] staging: unisys: use common return path

[Sudip Mukherjee]

On Tue, Dec 01, 2015 at 06:57:24PM +0300, Dan Carpenter wrote:
> On Tue, Dec 01, 2015 at 09:54:50AM -0500, Ben Romer wrote:
> > On 12/01/2015 03:00 AM, Dan Carpenter wrote:
> > >Doing One Err style error handling is often a mistake but it's ok here.
> > 
> > Why is it okay here? I don't understand why this function would be
> > any different than the other places where the code used a goto.
> 
> What I meant was that I'm generally opposed to "common exit paths".
> Mixing all the exit paths together often makes the code more complicated
> and leads to errors.  That makes sense from a common sense perspective

By "common exit path" do you mean mixing up of success and error paths?

regards
sudip

Re: [PATCH] staging: unisys: use common return path

[Dan Carpenter]

On Wed, Dec 02, 2015 at 10:32:49AM +0530, Sudip Mukherjee wrote:
> By "common exit path" do you mean mixing up of success and error paths?


Normally it's just the error paths which are mixed together.  Back in
the day, this unisys driver had tons of error handling bugs so that I
didn't even bother reporting them all.

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2015-May/068932.html
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2015-May/069073.html

regards,
dan carpenter