Re: transport_class: BUG if we can't release the attribute container

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Wed, 2008-04-02 at 08:30 -0600, Matthew Wilcox wrote:
> On Wed, Apr 02, 2008 at 09:15:53AM -0500, James Bottomley wrote:
> > On Tue, 2008-04-01 at 23:32 -0700, Greg KH wrote:
> > > BUG_ON() should not do anything in the macro except test for a value, no
> > > function calling.  I think checkpatch.pl checks for this...
> > 
> > Well, we can agree to differ on this.  The camp that wants no side
> > effects for BUG_ON() does so in case they want to define it to be a nop.
> 
> That's one argument, but to me, the most important thing is that reading
> the content of BUG_ON is unnecessary for understanding the function.
> 
> > OK ... your subsystem tree your call, I suppose.  How about the
> > attached.
> 
> > -static inline int transport_container_unregister(struct transport_container *tc)
> > +static inline void transport_container_unregister(struct transport_container *tc)
> >  {
> > -	return attribute_container_unregister(&tc->ac);
> > +	int err = attribute_container_unregister(&tc->ac);
> > +	BUG_ON(err);
> >  }
> 
> What's wrong with:
> 
> 	if (attribute_container_unregister(&tc->ac))
> 		BUG();

You've lost the unlikely designation which is one of the main reasons
for using BUG_ON().  Most people who write in this form also forget it
leading to a heap of suboptimal jump prediction code in the kernel and
another reason not to encourage it.

James