On Wed, Mar 09, 2016 at 05:10:28PM +0000, Sell, Timothy C wrote: > > -----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". I don't want to take a patch now that makes it harder to fix a real bug just to make a mythical future patch somehow better/smaller. Please fix this issue now. I'm dropping this series from my queue, please fix up and resend. thanks, greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
