| Message ID | 1386680134-85452-7-git-send-email-yangyingliang@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> #define SKIP_NONLOCAL(skb) \ > +({ \ > if (unlikely(skb->sk == NULL)) { \ > *err = -1; \ > return; \ > - } > + } \ > +}) That one should be lined up against the fence and shot :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/12/10 21:01, David Laight wrote: >> #define SKIP_NONLOCAL(skb) \ >> +({ \ >> if (unlikely(skb->sk == NULL)) { \ >> *err = -1; \ >> return; \ >> - } >> + } \ >> +}) > > That one should be lined up against the fence and shot :-) > > Hmm, not fully understand, did you mean that: #define SKIP_NONLOCAL(skb) ({ \ if (unlikely(skb->sk == NULL)) { \ *err = -1; \ return; \ } \ }) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Yang Yingliang <yangyingliang@huawei.com> Date: Tue, 10 Dec 2013 20:55:34 +0800 > Macros with complex values should be enclosed in parenthesis > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > net/sched/em_meta.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c > index e5cef956..852cd62 100644 > --- a/net/sched/em_meta.c > +++ b/net/sched/em_meta.c > @@ -272,10 +272,12 @@ META_COLLECTOR(int_rtiif) > **************************************************************************/ > > #define SKIP_NONLOCAL(skb) \ > +({ \ > if (unlikely(skb->sk == NULL)) { \ > *err = -1; \ > return; \ > - } > + } \ > +}) I can't apply this. First of all, "({ })" is for statements that evaluate to an lvalue, this macro does not. Second of all, and more importantly, this macro needs to be eliminated entirely. It completely hides control flow, and in the past we've killed macros which do this such as the old netlink attribute builders. The control flow should be inlined and expanded explicitly in the code so that someone who reads it can tell the control flow can be changed by the statement. Compare: SKIP_NONLOCAL(skb) to: if (skip_nonlocal(skb)) { *err = -1; return; } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of David Miller > From: Yang Yingliang <yangyingliang@huawei.com> > Date: Tue, 10 Dec 2013 20:55:34 +0800 > > > Macros with complex values should be enclosed in parenthesis > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > --- > > net/sched/em_meta.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c > > index e5cef956..852cd62 100644 > > --- a/net/sched/em_meta.c > > +++ b/net/sched/em_meta.c > > @@ -272,10 +272,12 @@ META_COLLECTOR(int_rtiif) > > **************************************************************************/ > > > > #define SKIP_NONLOCAL(skb) \ > > +({ \ > > if (unlikely(skb->sk == NULL)) { \ > > *err = -1; \ > > return; \ > > - } > > + } \ > > +}) > > I can't apply this. > > First of all, "({ })" is for statements that evaluate to an lvalue, > this macro does not. > > Second of all, and more importantly, this macro needs to be eliminated > entirely. It completely hides control flow, and in the past we've > killed macros which do this such as the old netlink attribute > builders. > > The control flow should be inlined and expanded explicitly in the > code so that someone who reads it can tell the control flow can > be changed by the statement. Compare: > > SKIP_NONLOCAL(skb) > > to: > > if (skip_nonlocal(skb)) { > *err = -1; > return; > } Or: if (!skb->sk) goto no_sk; ... return; no_sk: *err = -1; return; Actually WTF is a void function doing having an 'int *err' argument? These should just be: if (!skb->sk) return -1; David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c index e5cef956..852cd62 100644 --- a/net/sched/em_meta.c +++ b/net/sched/em_meta.c @@ -272,10 +272,12 @@ META_COLLECTOR(int_rtiif) **************************************************************************/ #define SKIP_NONLOCAL(skb) \ +({ \ if (unlikely(skb->sk == NULL)) { \ *err = -1; \ return; \ - } + } \ +}) META_COLLECTOR(int_sk_family) {
Macros with complex values should be enclosed in parenthesis Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/sched/em_meta.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)