Re spaghetti: I certainly agree that most uses of 'goto' are evil. But I've found that when 'goto' is used to provide a single common exit-point for a function (e.g., "goto away"), it can lead to code that is much more readable and much less fragile than the alternative of sprinkling 'return's at various points within the function, especially when different clean-up actions are needed prior to each of those 'return's. For me, knowing that there is exactly one place where a function can return (i.e., at the end), and exactly one place where we perform resource clean-ups (i.e., at the 'away' label) makes it easier to write correct code, and I think easier to understand. 'away' essentially fills a destructor-like role. I don't consider such a use of 'goto' as 'spaghetti code'. I agree that in trivial functions, this strategy would be overkill, and I suspect this may indeed be what is at the core of your argument against it. > -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Friday, February 12, 2016 5:02 PM > To: Romer, Benjamin M > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; > Sell, Timothy C; *S-Par-Maintainer > Subject: Re: Time for a code audit? > > I looked at the Smatch warnings, plus some bonus stuff I'm still working > on. > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() > warn: XXX should 'inp_pfn + i' be a 64 bit type? > > This is because the left side is u64 but we wrap at u32. > > drivers/staging/unisys/visorbus/visorbus_main.c:787 > visordriver_probe_device() warn: 'sem:&dev->visordriver_callback_lock' is > sometimes locked here and sometimes unlocked. > drivers/staging/unisys/visorbus/visorchipset.c:542 toolaction_show() warn: > XXX passing uninitialized 'tool_action' > drivers/staging/unisys/visorbus/visorchipset.c:609 error_show() warn: XXX > passing uninitialized 'error' > drivers/staging/unisys/visorbus/visorchipset.c:639 textid_show() warn: XXX > passing uninitialized 'text_id' > drivers/staging/unisys/visorbus/visorchipset.c:669 remaining_steps_show() > warn: XXX passing uninitialized 'remaining_steps' > > These with XXX are if channel->nbytes is too small. > > drivers/staging/unisys/visorbus/visorchipset.c:2217 visorchipset_ioctl() > warn: user controlled 'adjustment' cast to postive rl = 's64min-s64max' > > This is because we read a s64 and pass it to a function as u64. > > Do an: grep "rc = -1" drivers/staging/unisys/ -R > > Also "if (rc != 0)" is a double negative. != zero is good style when > you are talking about the number zero or for strcmp() functions. > > Ho ho ho. > int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev- > >devnodes[0]); > > You guys know that when I read > drivers/staging/unisys/visorbus/visorbus_main.c there is still a lot > that makes me itch. All the unnecessary initialization to -1 and the > goto aways. > > char s[99]; Why 99? Why s? I'm not a fan of a lot of the naming. > What is s? What is x? > > Blank lines after declaration sections. > > 781 fix_vbus_dev_info(dev); > 782 up(&dev->visordriver_callback_lock); > 783 rc = 0; > 784 away: > 785 if (rc != 0) > 786 put_device(&dev->device); > 787 return rc; > 788 } > > This is like in my kitchen because when I'm making spaghetti I throw > some against the wall to see if it is done and eventual the whole wall > has tiny spots of pasta stuck to it. > > I'm half way through the first file and this code is *almost* so close > to being ready, but could you just go through it once more and do a > final tidy up. > > regards, > dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
