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

[Dan Carpenter <dan.carpenter@oracle.com>]

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