On Fri, Aug 19, 2016 at 11:06:16AM -0400, Chuck Lever wrote: > > On Aug 19, 2016, at 10:50 AM, J. Bruce Fields <bfields@xxxxxxxxxx> wrote: > > On Thu, Aug 18, 2016 at 06:11:43PM -0400, Chuck Lever wrote: > >> OK, but why is a WARN_ON needed here? Why not return -EINVAL, > >> for example (once you've corrected BC_TCP -> BC) ? > > > > Well, it would be a programming bug, so I'd want a WARN_ON or similar > > somewhere, I don't care particularly where it is if you see a better way > > to organize things. > > The way it works now, the WARN_ON fires, but the logic goes ahead > and creates the transport anyway. > > If this is a programming bug, it should fail and return an error, I haven't been following that rule. Once upon a time, I would have put a BUG() there. Then Linus pointed out that sometimes a BUG() can bork the machine badly enough that the backtrace doesn't even make it to the logs, rendering it useless. (And I believe that could be the case here since this is running as a work item.) So, I stick a WARN() there instead and don't worry much what happens afterwards. > If it is not a programming bug (which is implied by the fact that > a transport is created anyway) then no WARN_ON is needed. So, could we just agree that WARN_ON means "there's a programming error", regardless of what happens next? Backtraces should never happen on a working kernel. And then ignore the following code path. Unless it's something that's obviously going to immediately oops in the warned case, in which case if we really want the warning then we should return if that looks safer. But I don't have really strong feelings about this case, the warning may be academic since setup_callback_client() makes this look obviously impossible, so if you want to reorganize this somehow, feel free to give it a shot. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
