Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG

[Gustavo A. R. Silva]

Quoting "Gustavo A. R. Silva" <garsilva@xxxxxxxxxxxxxx>:

> Hi all,
>
> Quoting Steffen Klassert <steffen.klassert@xxxxxxxxxxx>:
>
> > On Wed, Oct 25, 2017 at 01:22:22PM +0900, David Miller wrote:
> > > From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> > > Date: Wed, 25 Oct 2017 12:05:41 +0800
> > >
> > > > On Tue, Oct 24, 2017 at 05:48:42PM +0900, David Miller wrote:
> > > > > This discussion has happened before.
> > > > >
> > > > > But I'll explain the conclusion here for your benefit.
> > > > >
> > > > > BUG_ON() is a statement and everything inside of it will
> > > > > always execute.
> > > > >
> > > > > BUG_ON() is always preferred because it allows arch
> > > > > specific code to pass the conditional result properly
> > > > > into inline asm and builtins for optimal code generation.
> > > >
> > > > This is a good point.  However, while a little bit more verbose you
> > > > can still achieve the same assembly-level result by something like
> > > >
> > > > 	int err;
> > > >
> > > > 	err = <insert real code here>;
> > > > 	BUG_ON(err);
> > > >
> > > > Having real code in BUG_ON may pose problems to people reading the
> > > > code because some of us tend to ignore code in BUG_ON and similar
> > > > macros such as BUILD_BUG_ON.
> > >
> > > I agree that this makes the code easier to read and audit.
> >
> > It seems that we have an agreement on the above version,
> > Gustavo can you please update your patches to this?

By the way... this solution applies to the following sort of code:

if (xdr_buf_subsegment(buf, &integ_buf, 0, integ_len))
	BUG();

But what about the original code in this patch:

if (build_spdinfo(r_skb, net, sportid, seq, *flags) < 0)
	BUG();

I don't think we want something like:

bool err;

err = build_spdinfo(r_skb, net, sportid, seq, *flags) < 0 ? true : false;
BUG_ON(err);

Are you willing to accept the original patch in this case?

Thanks
--
Gustavo A. R. Silva