Re: WARN_ON added to rpc_create()

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

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@fieldses.org> 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@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.
> 
> 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?