RE: [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis

["David Laight" <David.Laight@ACULAB.COM>]

> 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