Re: WARN_ON added to rpc_create()

["J. Bruce Fields" <bfields@fieldses.org>]

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@redhat.com> 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.