On Mon, Feb 15, 2016 at 05:35:01PM +0000, Sell, Timothy C wrote: > 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. I've written a lot about error handling. https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ Do everything labels are the most error prone way of doing error handling. I discovered this because I look at a lot of error handling code but it makes sense because doing everything is more complicated than doing one thing. I have also looked at does one err style error handling prevent future bugs. For example, when people introduce a new return -ENOMEM; without dropping the lock, does it matter if there is an out: label at the bottom of the function and basically it does not. The people who don't care about locks don't care about common exit paths either. This style also causes future bugs when we change from a NULL on error kmalloc(), copy_from_user() to an error pointer function like kmemdup_user(). If you use canonical unwinding then you don't have that problem because you don't free things which have not been allocated. One err style causes bugs in the present and it causes bugs in the future. And empirically, historically it doesn't prevented people from adding new returns in the middle of functions. > > 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 } What makes this spaghetti code is that we twist the error paths and the success paths together. I put it in my email because it was ugly, but looking at it now I see it's also buggy. It's hard to argue this prevents future bugs when it is buggy right now as-is. Of course, that's a sample size of one. Let's look at the rest of the file. visorbus_match(), it's impossible to tell if the error codes are intentional which is typical for do-nothing gotos. The comment is not totally helpful. 343 away: 344 if (rc < 0) { 345 kfree(myattr); 346 myattr = NULL; 347 dev->devnodes[slot].attr = NULL; 348 } 349 return rc; 350 } Memory corruption. Gar, Smatch should have caught this. create_visor_device() has incomplete error handling. 1460 rc = 0; 1461 away: 1462 if (rc < 0) { 1463 if (notify_func) 1464 (*notify_func)(dev, rc); 1465 } 1466 } Hm... This one is tricky. rc is -1. notify_func is either visorchipset_device_pause_response() or device_resume_response(). I'm a bit confused about ->pending_msg_hdr and how that is protected from races. We do "msg->hdr.completion_status = (u32)(-response);" where response is -1. It's hard to tell what's going on here. I think normally we pass positive error codes to response like CONTROLVM_RESP_ERROR_DEVICE_UDEV_TIMEOUT (1400)? That's the end of the file. So most of the time if the away label does something more complicated than printing a message and and returning it's probably buggy. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
