* [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
@ 2017-10-23 18:18 Gustavo A. R. Silva
2017-10-24 3:25 ` Herbert Xu
0 siblings, 1 reply; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-23 18:18 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu, David S. Miller
Cc: netdev, linux-kernel, Gustavo A. R. Silva
Use BUG_ON instead of if condition followed by BUG.
This issue was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
net/xfrm/xfrm_user.c | 28 ++++++++++------------------
1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 465f3ec..9e8851f 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1152,8 +1152,7 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
if (r_skb == NULL)
return -ENOMEM;
- if (build_spdinfo(r_skb, net, sportid, seq, *flags) < 0)
- BUG();
+ BUG_ON(build_spdinfo(r_skb, net, sportid, seq, *flags) < 0);
return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
}
@@ -1210,8 +1209,7 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
if (r_skb == NULL)
return -ENOMEM;
- if (build_sadinfo(r_skb, net, sportid, seq, *flags) < 0)
- BUG();
+ BUG_ON(build_sadinfo(r_skb, net, sportid, seq, *flags) < 0);
return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
}
@@ -1958,8 +1956,8 @@ static int xfrm_get_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
c.seq = nlh->nlmsg_seq;
c.portid = nlh->nlmsg_pid;
- if (build_aevent(r_skb, x, &c) < 0)
- BUG();
+ BUG_ON(build_aevent(r_skb, x, &c) < 0);
+
err = nlmsg_unicast(net->xfrm.nlsk, r_skb, NETLINK_CB(skb).portid);
spin_unlock_bh(&x->lock);
xfrm_state_put(x);
@@ -2393,8 +2391,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
return -ENOMEM;
/* build migrate */
- if (build_migrate(skb, m, num_migrate, k, sel, encap, dir, type) < 0)
- BUG();
+ BUG_ON(build_migrate(skb, m, num_migrate, k, sel, encap, dir, type) < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
}
@@ -2623,8 +2620,7 @@ static int xfrm_aevent_state_notify(struct xfrm_state *x, const struct km_event
if (skb == NULL)
return -ENOMEM;
- if (build_aevent(skb, x, c) < 0)
- BUG();
+ BUG_ON(build_aevent(skb, x, c) < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_AEVENTS);
}
@@ -2836,8 +2832,7 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
if (skb == NULL)
return -ENOMEM;
- if (build_acquire(skb, x, xt, xp) < 0)
- BUG();
+ BUG_ON(build_acquire(skb, x, xt, xp) < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_ACQUIRE);
}
@@ -2951,8 +2946,7 @@ static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, const struct
if (skb == NULL)
return -ENOMEM;
- if (build_polexpire(skb, xp, dir, c) < 0)
- BUG();
+ BUG_ON(build_polexpire(skb, xp, dir, c) < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_EXPIRE);
}
@@ -3112,8 +3106,7 @@ static int xfrm_send_report(struct net *net, u8 proto,
if (skb == NULL)
return -ENOMEM;
- if (build_report(skb, proto, sel, addr) < 0)
- BUG();
+ BUG_ON(build_report(skb, proto, sel, addr) < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_REPORT);
}
@@ -3165,8 +3158,7 @@ static int xfrm_send_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr,
if (skb == NULL)
return -ENOMEM;
- if (build_mapping(skb, x, ipaddr, sport) < 0)
- BUG();
+ BUG_ON(build_mapping(skb, x, ipaddr, sport) < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MAPPING);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-23 18:18 [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG Gustavo A. R. Silva
@ 2017-10-24 3:25 ` Herbert Xu
2017-10-24 3:50 ` Gustavo A. R. Silva
2017-10-24 8:47 ` [PATCH] " David Miller
0 siblings, 2 replies; 19+ messages in thread
From: Herbert Xu @ 2017-10-24 3:25 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Steffen Klassert, David S. Miller, netdev, linux-kernel
On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
> Use BUG_ON instead of if condition followed by BUG.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
I think this patch is terrible. Why on earth is Coccinelle even
warning about this?
If anything we should be converting these constructs to not use
BUG.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 3:25 ` Herbert Xu
@ 2017-10-24 3:50 ` Gustavo A. R. Silva
2017-10-24 3:53 ` Herbert Xu
2017-10-24 8:47 ` [PATCH] " David Miller
1 sibling, 1 reply; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-24 3:50 UTC (permalink / raw)
To: Herbert Xu; +Cc: Steffen Klassert, David S. Miller, netdev, linux-kernel
Quoting Herbert Xu <herbert@gondor.apana.org.au>:
> On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
>> Use BUG_ON instead of if condition followed by BUG.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> I think this patch is terrible. Why on earth is Coccinelle even
> warning about this?
>
> If anything we should be converting these constructs to not use
> BUG.
>
Yeah, and under this assumption the original code is even worse.
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 3:50 ` Gustavo A. R. Silva
@ 2017-10-24 3:53 ` Herbert Xu
2017-10-24 3:57 ` Gustavo A. R. Silva
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Herbert Xu @ 2017-10-24 3:53 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Steffen Klassert, David S. Miller, netdev, linux-kernel
On Mon, Oct 23, 2017 at 10:50:43PM -0500, Gustavo A. R. Silva wrote:
>
> Quoting Herbert Xu <herbert@gondor.apana.org.au>:
>
> >On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
> >>Use BUG_ON instead of if condition followed by BUG.
> >>
> >>This issue was detected with the help of Coccinelle.
> >>
> >>Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> >
> >I think this patch is terrible. Why on earth is Coccinelle even
> >warning about this?
> >
> >If anything we should be converting these constructs to not use
> >BUG.
> >
>
> Yeah, and under this assumption the original code is even worse.
No your patch makes it worse. The original construct makes it
clear that the conditional is real code and not just some debugging
aid.
With your patch, real code is now inside BUG_ON.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 3:53 ` Herbert Xu
@ 2017-10-24 3:57 ` Gustavo A. R. Silva
2017-10-24 4:01 ` Alexei Starovoitov
2017-10-24 8:48 ` David Miller
2 siblings, 0 replies; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-24 3:57 UTC (permalink / raw)
To: Herbert Xu; +Cc: Steffen Klassert, David S. Miller, netdev, linux-kernel
Quoting Herbert Xu <herbert@gondor.apana.org.au>:
> On Mon, Oct 23, 2017 at 10:50:43PM -0500, Gustavo A. R. Silva wrote:
>>
>> Quoting Herbert Xu <herbert@gondor.apana.org.au>:
>>
>> >On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
>> >>Use BUG_ON instead of if condition followed by BUG.
>> >>
>> >>This issue was detected with the help of Coccinelle.
>> >>
>> >>Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> >
>> >I think this patch is terrible. Why on earth is Coccinelle even
>> >warning about this?
>> >
>> >If anything we should be converting these constructs to not use
>> >BUG.
>> >
>>
>> Yeah, and under this assumption the original code is even worse.
>
> No your patch makes it worse. The original construct makes it
> clear that the conditional is real code and not just some debugging
> aid.
>
> With your patch, real code is now inside BUG_ON.
>
What is the reason for BUG_ON to exist then if it makes the code worse
than an _if_ condition followed by BUG?
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 3:53 ` Herbert Xu
2017-10-24 3:57 ` Gustavo A. R. Silva
@ 2017-10-24 4:01 ` Alexei Starovoitov
2017-10-24 4:30 ` Herbert Xu
2017-10-24 8:48 ` David Miller
2 siblings, 1 reply; 19+ messages in thread
From: Alexei Starovoitov @ 2017-10-24 4:01 UTC (permalink / raw)
To: Herbert Xu
Cc: Gustavo A. R. Silva, Steffen Klassert, David S. Miller, netdev,
linux-kernel
On Tue, Oct 24, 2017 at 11:53:20AM +0800, Herbert Xu wrote:
> On Mon, Oct 23, 2017 at 10:50:43PM -0500, Gustavo A. R. Silva wrote:
> >
> > Quoting Herbert Xu <herbert@gondor.apana.org.au>:
> >
> > >On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
> > >>Use BUG_ON instead of if condition followed by BUG.
> > >>
> > >>This issue was detected with the help of Coccinelle.
> > >>
> > >>Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> > >
> > >I think this patch is terrible. Why on earth is Coccinelle even
> > >warning about this?
> > >
> > >If anything we should be converting these constructs to not use
> > >BUG.
> > >
> >
> > Yeah, and under this assumption the original code is even worse.
>
> No your patch makes it worse. The original construct makes it
> clear that the conditional is real code and not just some debugging
> aid.
>
> With your patch, real code is now inside BUG_ON.
fwiw I had the same argument earlier:
https://lkml.org/lkml/2017/10/9/1139
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 4:01 ` Alexei Starovoitov
@ 2017-10-24 4:30 ` Herbert Xu
0 siblings, 0 replies; 19+ messages in thread
From: Herbert Xu @ 2017-10-24 4:30 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Gustavo A. R. Silva, Steffen Klassert, David S. Miller, netdev,
linux-kernel
On Mon, Oct 23, 2017 at 09:01:46PM -0700, Alexei Starovoitov wrote:
>
> fwiw I had the same argument earlier:
> https://lkml.org/lkml/2017/10/9/1139
Fair point on eliminating a branch. But I'd prefer something like
bool cond;
cond = code_that_does_something();
BUG_ON(cond);
rather than just
BUG_ON(code_that_does_something());
But maybe it's just me.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 3:25 ` Herbert Xu
2017-10-24 3:50 ` Gustavo A. R. Silva
@ 2017-10-24 8:47 ` David Miller
1 sibling, 0 replies; 19+ messages in thread
From: David Miller @ 2017-10-24 8:47 UTC (permalink / raw)
To: herbert; +Cc: garsilva, steffen.klassert, netdev, linux-kernel
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 24 Oct 2017 11:25:08 +0800
> On Mon, Oct 23, 2017 at 01:18:57PM -0500, Gustavo A. R. Silva wrote:
>> Use BUG_ON instead of if condition followed by BUG.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> I think this patch is terrible. Why on earth is Coccinelle even
> warning about this?
BUG_ON(x) generates significantly better code than
"if (cond) BUG();" because it's not possible to emit
the most optimal code sequences like on powerpc that has
conditional traps.
That's why.
I'm applying these transformations all over the networking as I
receive them and I advise that you do so for crypto as well.
Thank.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 3:53 ` Herbert Xu
2017-10-24 3:57 ` Gustavo A. R. Silva
2017-10-24 4:01 ` Alexei Starovoitov
@ 2017-10-24 8:48 ` David Miller
2017-10-25 4:05 ` Herbert Xu
2 siblings, 1 reply; 19+ messages in thread
From: David Miller @ 2017-10-24 8:48 UTC (permalink / raw)
To: herbert; +Cc: garsilva, steffen.klassert, netdev, linux-kernel
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 24 Oct 2017 11:53:20 +0800
> No your patch makes it worse. The original construct makes it
> clear that the conditional is real code and not just some debugging
> aid.
>
> With your patch, real code is now inside BUG_ON.
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-24 8:48 ` David Miller
@ 2017-10-25 4:05 ` Herbert Xu
2017-10-25 4:22 ` David Miller
0 siblings, 1 reply; 19+ messages in thread
From: Herbert Xu @ 2017-10-25 4:05 UTC (permalink / raw)
To: David Miller; +Cc: garsilva, steffen.klassert, netdev, linux-kernel
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.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-25 4:05 ` Herbert Xu
@ 2017-10-25 4:22 ` David Miller
2017-10-25 7:28 ` Steffen Klassert
0 siblings, 1 reply; 19+ messages in thread
From: David Miller @ 2017-10-25 4:22 UTC (permalink / raw)
To: herbert; +Cc: garsilva, steffen.klassert, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-25 4:22 ` David Miller
@ 2017-10-25 7:28 ` Steffen Klassert
2017-10-25 16:43 ` Gustavo A. R. Silva
0 siblings, 1 reply; 19+ messages in thread
From: Steffen Klassert @ 2017-10-25 7:28 UTC (permalink / raw)
To: David Miller; +Cc: herbert, garsilva, netdev, linux-kernel
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?
Thanks!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-25 7:28 ` Steffen Klassert
@ 2017-10-25 16:43 ` Gustavo A. R. Silva
2017-10-26 0:38 ` Gustavo A. R. Silva
0 siblings, 1 reply; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-25 16:43 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, herbert, netdev, linux-kernel
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?
>
I can do that. I'll work on a patchset for this.
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-25 16:43 ` Gustavo A. R. Silva
@ 2017-10-26 0:38 ` Gustavo A. R. Silva
2017-10-26 6:36 ` Herbert Xu
0 siblings, 1 reply; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-26 0:38 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, herbert, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-26 0:38 ` Gustavo A. R. Silva
@ 2017-10-26 6:36 ` Herbert Xu
2017-10-26 10:47 ` Gustavo A. R. Silva
0 siblings, 1 reply; 19+ messages in thread
From: Herbert Xu @ 2017-10-26 6:36 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Steffen Klassert, David Miller, netdev, linux-kernel
On Wed, Oct 25, 2017 at 07:38:35PM -0500, Gustavo A. R. Silva wrote:
>
> 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);
How about
int err;
err = build_spdinfo(r_skb, net, sportid, seq, *flags);
BUG_ON(err < 0);
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-26 6:36 ` Herbert Xu
@ 2017-10-26 10:47 ` Gustavo A. R. Silva
2017-10-26 11:31 ` [PATCH v2] " Gustavo A. R. Silva
0 siblings, 1 reply; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-26 10:47 UTC (permalink / raw)
To: Herbert Xu; +Cc: Steffen Klassert, David Miller, netdev, linux-kernel
Hi Herbert,
Quoting Herbert Xu <herbert@gondor.apana.org.au>:
> On Wed, Oct 25, 2017 at 07:38:35PM -0500, Gustavo A. R. Silva wrote:
>>
>> 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);
>
> How about
>
> int err;
>
> err = build_spdinfo(r_skb, net, sportid, seq, *flags);
> BUG_ON(err < 0);
>
I like it.
Thanks
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-26 10:47 ` Gustavo A. R. Silva
@ 2017-10-26 11:31 ` Gustavo A. R. Silva
2017-10-27 10:46 ` Steffen Klassert
0 siblings, 1 reply; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-26 11:31 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu, David S. Miller
Cc: netdev, linux-kernel, Gustavo A. R. Silva
Use BUG_ON instead of if condition followed by BUG.
This issue was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
Update the code as suggested by Herbert Xu:
int err;
err = foo();
BUG_ON(err < 0);
net/xfrm/xfrm_user.c | 45 +++++++++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b997f13..fb8c79e 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1147,13 +1147,14 @@ static int xfrm_get_spdinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
u32 *flags = nlmsg_data(nlh);
u32 sportid = NETLINK_CB(skb).portid;
u32 seq = nlh->nlmsg_seq;
+ int err;
r_skb = nlmsg_new(xfrm_spdinfo_msgsize(), GFP_ATOMIC);
if (r_skb == NULL)
return -ENOMEM;
- if (build_spdinfo(r_skb, net, sportid, seq, *flags) < 0)
- BUG();
+ err = build_spdinfo(r_skb, net, sportid, seq, *flags);
+ BUG_ON(err < 0);
return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
}
@@ -1205,13 +1206,14 @@ static int xfrm_get_sadinfo(struct sk_buff *skb, struct nlmsghdr *nlh,
u32 *flags = nlmsg_data(nlh);
u32 sportid = NETLINK_CB(skb).portid;
u32 seq = nlh->nlmsg_seq;
+ int err;
r_skb = nlmsg_new(xfrm_sadinfo_msgsize(), GFP_ATOMIC);
if (r_skb == NULL)
return -ENOMEM;
- if (build_sadinfo(r_skb, net, sportid, seq, *flags) < 0)
- BUG();
+ err = build_sadinfo(r_skb, net, sportid, seq, *flags);
+ BUG_ON(err < 0);
return nlmsg_unicast(net->xfrm.nlsk, r_skb, sportid);
}
@@ -1958,8 +1960,9 @@ static int xfrm_get_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
c.seq = nlh->nlmsg_seq;
c.portid = nlh->nlmsg_pid;
- if (build_aevent(r_skb, x, &c) < 0)
- BUG();
+ err = build_aevent(r_skb, x, &c);
+ BUG_ON(err < 0);
+
err = nlmsg_unicast(net->xfrm.nlsk, r_skb, NETLINK_CB(skb).portid);
spin_unlock_bh(&x->lock);
xfrm_state_put(x);
@@ -2386,6 +2389,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
{
struct net *net = &init_net;
struct sk_buff *skb;
+ int err;
skb = nlmsg_new(xfrm_migrate_msgsize(num_migrate, !!k, !!encap),
GFP_ATOMIC);
@@ -2393,8 +2397,8 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
return -ENOMEM;
/* build migrate */
- if (build_migrate(skb, m, num_migrate, k, sel, encap, dir, type) < 0)
- BUG();
+ err = build_migrate(skb, m, num_migrate, k, sel, encap, dir, type);
+ BUG_ON(err < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
}
@@ -2618,13 +2622,14 @@ static int xfrm_aevent_state_notify(struct xfrm_state *x, const struct km_event
{
struct net *net = xs_net(x);
struct sk_buff *skb;
+ int err;
skb = nlmsg_new(xfrm_aevent_msgsize(x), GFP_ATOMIC);
if (skb == NULL)
return -ENOMEM;
- if (build_aevent(skb, x, c) < 0)
- BUG();
+ err = build_aevent(skb, x, c);
+ BUG_ON(err < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_AEVENTS);
}
@@ -2830,13 +2835,14 @@ static int xfrm_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *xt,
{
struct net *net = xs_net(x);
struct sk_buff *skb;
+ int err;
skb = nlmsg_new(xfrm_acquire_msgsize(x, xp), GFP_ATOMIC);
if (skb == NULL)
return -ENOMEM;
- if (build_acquire(skb, x, xt, xp) < 0)
- BUG();
+ err = build_acquire(skb, x, xt, xp);
+ BUG_ON(err < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_ACQUIRE);
}
@@ -2945,13 +2951,14 @@ static int xfrm_exp_policy_notify(struct xfrm_policy *xp, int dir, const struct
{
struct net *net = xp_net(xp);
struct sk_buff *skb;
+ int err;
skb = nlmsg_new(xfrm_polexpire_msgsize(xp), GFP_ATOMIC);
if (skb == NULL)
return -ENOMEM;
- if (build_polexpire(skb, xp, dir, c) < 0)
- BUG();
+ err = build_polexpire(skb, xp, dir, c);
+ BUG_ON(err < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_EXPIRE);
}
@@ -3105,13 +3112,14 @@ static int xfrm_send_report(struct net *net, u8 proto,
struct xfrm_selector *sel, xfrm_address_t *addr)
{
struct sk_buff *skb;
+ int err;
skb = nlmsg_new(xfrm_report_msgsize(), GFP_ATOMIC);
if (skb == NULL)
return -ENOMEM;
- if (build_report(skb, proto, sel, addr) < 0)
- BUG();
+ err = build_report(skb, proto, sel, addr);
+ BUG_ON(err < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_REPORT);
}
@@ -3152,6 +3160,7 @@ static int xfrm_send_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr,
{
struct net *net = xs_net(x);
struct sk_buff *skb;
+ int err;
if (x->id.proto != IPPROTO_ESP)
return -EINVAL;
@@ -3163,8 +3172,8 @@ static int xfrm_send_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr,
if (skb == NULL)
return -ENOMEM;
- if (build_mapping(skb, x, ipaddr, sport) < 0)
- BUG();
+ err = build_mapping(skb, x, ipaddr, sport);
+ BUG_ON(err < 0);
return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MAPPING);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-26 11:31 ` [PATCH v2] " Gustavo A. R. Silva
@ 2017-10-27 10:46 ` Steffen Klassert
2017-10-27 20:56 ` Gustavo A. R. Silva
0 siblings, 1 reply; 19+ messages in thread
From: Steffen Klassert @ 2017-10-27 10:46 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
On Thu, Oct 26, 2017 at 06:31:35AM -0500, Gustavo A. R. Silva wrote:
> Use BUG_ON instead of if condition followed by BUG.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Applied to ipsec-next, thanks Gustavo!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] net: xfrm_user: use BUG_ON instead of if condition followed by BUG
2017-10-27 10:46 ` Steffen Klassert
@ 2017-10-27 20:56 ` Gustavo A. R. Silva
0 siblings, 0 replies; 19+ messages in thread
From: Gustavo A. R. Silva @ 2017-10-27 20:56 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
Quoting Steffen Klassert <steffen.klassert@secunet.com>:
> On Thu, Oct 26, 2017 at 06:31:35AM -0500, Gustavo A. R. Silva wrote:
>> Use BUG_ON instead of if condition followed by BUG.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>
> Applied to ipsec-next, thanks Gustavo!
Glad to help. :)
Thank you
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-10-27 20:56 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-23 18:18 [PATCH] net: xfrm_user: use BUG_ON instead of if condition followed by BUG Gustavo A. R. Silva
2017-10-24 3:25 ` Herbert Xu
2017-10-24 3:50 ` Gustavo A. R. Silva
2017-10-24 3:53 ` Herbert Xu
2017-10-24 3:57 ` Gustavo A. R. Silva
2017-10-24 4:01 ` Alexei Starovoitov
2017-10-24 4:30 ` Herbert Xu
2017-10-24 8:48 ` David Miller
2017-10-25 4:05 ` Herbert Xu
2017-10-25 4:22 ` David Miller
2017-10-25 7:28 ` Steffen Klassert
2017-10-25 16:43 ` Gustavo A. R. Silva
2017-10-26 0:38 ` Gustavo A. R. Silva
2017-10-26 6:36 ` Herbert Xu
2017-10-26 10:47 ` Gustavo A. R. Silva
2017-10-26 11:31 ` [PATCH v2] " Gustavo A. R. Silva
2017-10-27 10:46 ` Steffen Klassert
2017-10-27 20:56 ` Gustavo A. R. Silva
2017-10-24 8:47 ` [PATCH] " David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.