Messages in this thread | | | Date | Wed, 25 Oct 2017 19:38:35 -0500 | From | "Gustavo A. R. Silva" <> | Subject | Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG |
| |
Quoting "Gustavo A. R. Silva" <garsilva@embeddedor.com>:
> Hi all, > > Quoting Steffen Klassert <steffen.klassert@secunet.com>: > >> On Wed, Oct 25, 2017 at 01:22:22PM +0900, David Miller wrote: >>> From: Herbert Xu <herbert@gondor.apana.org.au> >>> 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
| |