On Thu, 24 Sep 2015, Dan Carpenter wrote: > Of course, the maintainer gets the last word regardless of what anyone > else thinks. > > Generally, minimal code is better. Trying to future proof code is a > waste of time because you can't predict what will happen in the future. > It's way more likely that some pointer you never expected to be NULL > will be NULL instead of the few checked at the beginning of a function. > Adding useless code uses RAM and makes the function slower. It's a bit > confusing for users as well because they will wonder when the NULL check > is used. A lot of times this sort of error handling is a bit fake and > what I mean is that it looks correct but the system will just crash in a > later function. > > Also especially with a simple NULL dereferences like this theoretical > one, it's better to just get the oops. It kills the module but you get > a good message in the log and it's normally straight forward to debug. > > We spent a surprising amount of time discussing useless code. I made > someone redo a patch yesterday because they had incomplete error > handling for a situation which could never happen. > > regards, > dan carpenter > > Thanks for the discussion. Interesting. The amount of code bloat here compiles down to about two machine instructions (in two places). Actually a little more since I should be using IS_ERR_OR_NULL. But the main question is whether I should do it at all. The behaviour I should drive here is that the user will do their own error checking. After they get a pointer to a FPGA manager using of_fpga_mgr_get(), they should check it and not assume that fpga_mgr_firmware_load() will do it for them, i.e. mgr = of_fpga_mgr_get(mgr_node); if (IS_ERR(mgr)) return PTR_ERR(mgr); fpga_mgr_firmware_load(mgr, flags, path); I could take out these NULL pointer checks and it won't hurt anything unless someone is just using the functions badly, in which case: kablooey. Alan _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
