> On Aug 19, 2016, at 11:47 AM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > 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. I still don't understand. If you would have put a BUG here, then why does this logic continue and create the transport anyway? Well, it's a nit, so I'll drop it. >> 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. Right, it's that obviously impossible part that made me wonder why there was a warning here in the first place. OK, the fix is to do BC_TCP -> BC and put our pencils down. Do you want me to send you a patch, or do you plan to take care of it? -- Chuck Lever -- 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
