> -----Original Message----- > From: Neil Horman [mailto:nhorman@xxxxxxxxxx] > Sent: Wednesday, March 09, 2016 1:48 PM > To: Sell, Timothy C > Cc: external-ges-unisys@xxxxxxxxxx; 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 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. > > > 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. (deleted external-ges-unisys@xxxxxxxxxx from the cc, as several of us have already gotten our hands slapped for mixing dist lists...) I agree; our goal is NOT to remove gotos/labels. We are just attempting to clean up our usages of gotos to be more in-line with kernel conventions. We have only been removing gotos/labels in cases where they naturally seem to disappear after we clean things up. This exercise has already uncovered more than 1 bug, so I think it was useful. I will refamiliarize myself with Documentation/CodingStyle chapter 7. Thanks Neil. Tim Sell > > 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
