[PATCH 3/6] staging: unisys: visorbus: git rid of gotos in devmajorminor_create_file

[Neil Horman]

On Wed, Mar 09, 2016 at 05:10:28PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman at redhat.com]
> > Sent: Wednesday, March 09, 2016 11:09 AM
> > To: external-ges-unisys at redhat.com
> > Cc: gregkh at linuxfoundation.org; driverdev-devel at linuxdriverproject.org;
> > *S-Par-Maintainer
> > Subject: Re: [PATCH 3/6] staging: unisys: visorbus: git rid of gotos in
> > devmajorminor_create_file
> > 
> > On Tue, Mar 08, 2016 at 08:22:45PM -0500, David Kershner wrote:
> > > The gotos in devmajorminor_create_file aren't needed, get rid of them.
> > >
> > > Signed-off-by: David Kershner <david.kershner at unisys.com>
> > > Signed-off-by: Timothy Sell <timothy.sell at unisys.com>
> > > ---
> > >  drivers/staging/unisys/visorbus/visorbus_main.c | 25 ++++++++-------------
> > ----
> > >  1 file changed, 8 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> > b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > index e1ec0b8..37a60ec 100644
> > > --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> > > +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > @@ -306,21 +306,17 @@ devmajorminor_create_file(struct visor_device
> > *dev, const char *name,
> > >  {
> > >  	int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev-
> > >devnodes[0]);
> > >  	struct devmajorminor_attribute *myattr = NULL;
> > > -	int x = -1, rc = 0, slot = -1;
> > > +	int x = -1, slot = -1;
> > >
> > >  	register_devmajorminor_attributes(dev);
> > >  	for (slot = 0; slot < maxdevnodes; slot++)
> > >  		if (!dev->devnodes[slot].attr)
> > >  			break;
> > > -	if (slot == maxdevnodes) {
> > > -		rc = -ENOMEM;
> > > -		goto away;
> > > -	}
> > > +	if (slot == maxdevnodes)
> > > +		return -ENOMEM;
> > >  	myattr = kzalloc(sizeof(*myattr), GFP_KERNEL);
> > > -	if (!myattr) {
> > > -		rc = -ENOMEM;
> > > -		goto away;
> > > -	}
> > > +	if (!myattr)
> > > +		return -ENOMEM;
> > >  	myattr->show = DEVMAJORMINOR_ATTR;
> > >  	myattr->store = NULL;
> > >  	myattr->slot = slot;
> > > @@ -331,17 +327,12 @@ devmajorminor_create_file(struct visor_device
> > *dev, const char *name,
> > >  	dev->devnodes[slot].minor = minor;
> > >  	x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
> > >  	if (x < 0) {
> > > -		rc = x;
> > > -		goto away;
> > > -	}
> > > -	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > -away:
> > > -	if (rc < 0) {
> > >  		kfree(myattr);
> > > -		myattr = NULL;
> > >  		dev->devnodes[slot].attr = NULL;
> > > +		return x;
> > >  	}
> > > -	return rc;
> > > +	kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> > > +	return 0;
> > >  }
> > >
> > >  static void
> > > --
> > > 1.9.1
> > >
> > 
> > This problem wasn't introduced by this patch, but removing the gotos makes
> > it
> > harder to fix.  The first thing you do is call
> > register_devmajorminor_attributes, and if the code fails, you should really
> > unregister those.  What you really need to do is keep the label and gotos,
> > and
> > add an unregister call at the same place you use the kfree call
> > 
> > Neil
> 
> As part of our code audit to get this code out of staging,
> Dan Carpenter has expressed some criticims regarding our use of gotos,
> which included our use of vague label names like "away", and many places
> were we have a single exit-point for both success and failure exits.
> (I believe Dan has researched this and determined this practice to be
> error-prone.)  We have been attempting to clean these up, and as a result
> have found some places where it is cleaner to just remove the gotos
> altogether.  It looks like this was one of those places.
> 
ok, but I don't think dans criticism of your label names needs to translate into
a wholesale removal of goto statements.  Its very common practice to use (when
done appropriately). In fact, Documentation/CodingStyle has an entire chapter
(chapter 7) on the cetralization of function exit paths implemented with goto,
with very valid rationale, and I think Dan would agree with it.

Neil

> I see your point about register_devmajorminor_attributes, which is valid,
> but we just so-happen to have a subsequent patch that removes all
> of the devmajorminor stuff, as we don't really need that.  This was a
> result of criticism from Greg about our usages of "raw kobjects".
> 
> Tim Sell