On Fri, Aug 19, 2016 at 11:51:13AM -0400, Chuck Lever wrote: > > > 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? It seemed worth 1 line of screen real estate to say "this should never happen", but not 3? > Well, it's a nit, so I'll drop it. But, I mean, I don't care that much either. > >> 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 That would be great.--b. > or do you plan to take care of it? -- 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
