Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754895AbbLAP5m (ORCPT ); Tue, 1 Dec 2015 10:57:42 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:16423 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752619AbbLAP5l (ORCPT ); Tue, 1 Dec 2015 10:57:41 -0500 Date: Tue, 1 Dec 2015 18:57:24 +0300 From: Dan Carpenter To: Ben Romer Cc: Sudip Mukherjee , devel@driverdev.osuosl.org, Greg Kroah-Hartman , sparmaintainer@unisys.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] staging: unisys: use common return path Message-ID: <20151201155724.GG18797@mwanda> References: <1448950555-8846-1-git-send-email-sudipm.mukherjee@gmail.com> <20151201080045.GE18797@mwanda> <565DB4BA.80208@unisys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <565DB4BA.80208@unisys.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2753 Lines: 79 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/