> -----Original Message----- > From: Neil Horman [mailto:nhorman@xxxxxxxxxx] > Sent: Wednesday, March 09, 2016 11:09 AM > To: external-ges-unisys@xxxxxxxxxx > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; > *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@xxxxxxxxxx> > > Signed-off-by: Timothy Sell <timothy.sell@xxxxxxxxxx> > > --- > > 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. 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
