* [PATCH net-next 1/6] net_sched: remove unnecessary parentheses while return
2013-12-10 12:55 [PATCH net-next 0/6] net_sched: clean up some chekpatch errors Yang Yingliang
@ 2013-12-10 12:55 ` Yang Yingliang
2013-12-11 3:46 ` David Miller
2013-12-10 12:55 ` [PATCH net-next 2/6] net_sched: cls_bpf: use tabs to do indent Yang Yingliang
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Yang Yingliang @ 2013-12-10 12:55 UTC (permalink / raw)
To: davem, netdev
return is not a function, parentheses are not required.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/sched/act_api.c | 3 ++-
net/sched/sch_dsmark.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index fd70728..92a3c31 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -191,7 +191,8 @@ u32 tcf_hash_new_index(u32 *idx_gen, struct tcf_hashinfo *hinfo)
val = 1;
} while (tcf_hash_lookup(val, hinfo));
- return (*idx_gen = val);
+ *idx_gen = val;
+ return val;
}
EXPORT_SYMBOL(tcf_hash_new_index);
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 3886365..0952fd2 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -47,7 +47,7 @@ struct dsmark_qdisc_data {
static inline int dsmark_valid_index(struct dsmark_qdisc_data *p, u16 index)
{
- return (index <= p->indices && index > 0);
+ return index <= p->indices && index > 0;
}
/* ------------------------- Class/flow operations ------------------------- */
--
1.8.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next 2/6] net_sched: cls_bpf: use tabs to do indent
2013-12-10 12:55 [PATCH net-next 0/6] net_sched: clean up some chekpatch errors Yang Yingliang
2013-12-10 12:55 ` [PATCH net-next 1/6] net_sched: remove unnecessary parentheses while return Yang Yingliang
@ 2013-12-10 12:55 ` Yang Yingliang
2013-12-11 3:46 ` David Miller
2013-12-10 12:55 ` [PATCH net-next 3/6] net_sched: change "foo* bar" to "foo *bar" Yang Yingliang
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Yang Yingliang @ 2013-12-10 12:55 UTC (permalink / raw)
To: davem, netdev
Code indent should use tabs where possible
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/sched/cls_bpf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 1002a82..d7c72be 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -323,7 +323,7 @@ static int cls_bpf_dump(struct tcf_proto *tp, unsigned long fh,
if (nla == NULL)
goto nla_put_failure;
- memcpy(nla_data(nla), prog->bpf_ops, nla_len(nla));
+ memcpy(nla_data(nla), prog->bpf_ops, nla_len(nla));
if (tcf_exts_dump(skb, &prog->exts, &bpf_ext_map) < 0)
goto nla_put_failure;
--
1.8.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next 3/6] net_sched: change "foo* bar" to "foo *bar"
2013-12-10 12:55 [PATCH net-next 0/6] net_sched: clean up some chekpatch errors Yang Yingliang
2013-12-10 12:55 ` [PATCH net-next 1/6] net_sched: remove unnecessary parentheses while return Yang Yingliang
2013-12-10 12:55 ` [PATCH net-next 2/6] net_sched: cls_bpf: use tabs to do indent Yang Yingliang
@ 2013-12-10 12:55 ` Yang Yingliang
2013-12-11 3:46 ` David Miller
2013-12-10 12:55 ` [PATCH net-next 4/6] net_sched: add space around '>' and before '(' Yang Yingliang
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Yang Yingliang @ 2013-12-10 12:55 UTC (permalink / raw)
To: davem, netdev
"foo* bar" or "foo * bar" should be "foo *bar".
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/sched/cls_u32.c | 2 +-
net/sched/sch_generic.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index eb07a1e..59e546c 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -352,7 +352,7 @@ static int u32_destroy_key(struct tcf_proto *tp, struct tc_u_knode *n)
return 0;
}
-static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode* key)
+static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
{
struct tc_u_knode **kp;
struct tc_u_hnode *ht = key->ht_up;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 922a094..6a91d7d 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -338,13 +338,13 @@ EXPORT_SYMBOL(netif_carrier_off);
cheaper.
*/
-static int noop_enqueue(struct sk_buff *skb, struct Qdisc * qdisc)
+static int noop_enqueue(struct sk_buff *skb, struct Qdisc *qdisc)
{
kfree_skb(skb);
return NET_XMIT_CN;
}
-static struct sk_buff *noop_dequeue(struct Qdisc * qdisc)
+static struct sk_buff *noop_dequeue(struct Qdisc *qdisc)
{
return NULL;
}
--
1.8.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next 4/6] net_sched: add space around '>' and before '('
2013-12-10 12:55 [PATCH net-next 0/6] net_sched: clean up some chekpatch errors Yang Yingliang
` (2 preceding siblings ...)
2013-12-10 12:55 ` [PATCH net-next 3/6] net_sched: change "foo* bar" to "foo *bar" Yang Yingliang
@ 2013-12-10 12:55 ` Yang Yingliang
2013-12-11 3:46 ` David Miller
2013-12-10 12:55 ` [PATCH net-next 5/6] net_sched: sfq: put sfq_unlink in a do - while loop Yang Yingliang
2013-12-10 12:55 ` [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis Yang Yingliang
5 siblings, 1 reply; 18+ messages in thread
From: Yang Yingliang @ 2013-12-10 12:55 UTC (permalink / raw)
To: davem, netdev
Spaces required around that '>' (ctx:VxV) and
before the open parenthesis '('.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/sched/sch_cbq.c | 3 ++-
net/sched/sch_netem.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 7a42c81..d5a8a4b 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1058,7 +1058,8 @@ static void cbq_normalize_quanta(struct cbq_sched_data *q, int prio)
cl->quantum = (cl->weight*cl->allot*q->nclasses[prio])/
q->quanta[prio];
}
- if (cl->quantum <= 0 || cl->quantum>32*qdisc_dev(cl->qdisc)->mtu) {
+ if (cl->quantum <= 0 ||
+ cl->quantum > 32*qdisc_dev(cl->qdisc)->mtu) {
pr_warning("CBQ: class %08x has bad quantum==%ld, repaired.\n",
cl->common.classid, cl->quantum);
cl->quantum = qdisc_dev(cl->qdisc)->mtu/2 + 1;
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index bccd52b..f3befd6 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -729,7 +729,7 @@ static int get_loss_clg(struct Qdisc *sch, const struct nlattr *attr)
nla_for_each_nested(la, attr, rem) {
u16 type = nla_type(la);
- switch(type) {
+ switch (type) {
case NETEM_LOSS_GI: {
const struct tc_netem_gimodel *gi = nla_data(la);
--
1.8.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next 5/6] net_sched: sfq: put sfq_unlink in a do - while loop
2013-12-10 12:55 [PATCH net-next 0/6] net_sched: clean up some chekpatch errors Yang Yingliang
` (3 preceding siblings ...)
2013-12-10 12:55 ` [PATCH net-next 4/6] net_sched: add space around '>' and before '(' Yang Yingliang
@ 2013-12-10 12:55 ` Yang Yingliang
2013-12-11 3:47 ` David Miller
2013-12-10 12:55 ` [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis Yang Yingliang
5 siblings, 1 reply; 18+ messages in thread
From: Yang Yingliang @ 2013-12-10 12:55 UTC (permalink / raw)
To: davem, netdev
Macros with multiple statements should be enclosed in a do - while loop
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/sched/sch_sfq.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index d3a1bc2..76f01e0 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -237,10 +237,12 @@ static inline void sfq_link(struct sfq_sched_data *q, sfq_index x)
}
#define sfq_unlink(q, x, n, p) \
- n = q->slots[x].dep.next; \
- p = q->slots[x].dep.prev; \
- sfq_dep_head(q, p)->next = n; \
- sfq_dep_head(q, n)->prev = p
+ do { \
+ n = q->slots[x].dep.next; \
+ p = q->slots[x].dep.prev; \
+ sfq_dep_head(q, p)->next = n; \
+ sfq_dep_head(q, n)->prev = p; \
+ } while (0)
static inline void sfq_dec(struct sfq_sched_data *q, sfq_index x)
--
1.8.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis
2013-12-10 12:55 [PATCH net-next 0/6] net_sched: clean up some chekpatch errors Yang Yingliang
` (4 preceding siblings ...)
2013-12-10 12:55 ` [PATCH net-next 5/6] net_sched: sfq: put sfq_unlink in a do - while loop Yang Yingliang
@ 2013-12-10 12:55 ` Yang Yingliang
2013-12-10 13:01 ` David Laight
2013-12-11 3:49 ` David Miller
5 siblings, 2 replies; 18+ messages in thread
From: Yang Yingliang @ 2013-12-10 12:55 UTC (permalink / raw)
To: davem, netdev
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; \
- }
+ } \
+})
META_COLLECTOR(int_sk_family)
{
--
1.8.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* RE: [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis
2013-12-10 12:55 ` [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis Yang Yingliang
@ 2013-12-10 13:01 ` David Laight
2013-12-11 2:16 ` Yang Yingliang
2013-12-11 3:49 ` David Miller
1 sibling, 1 reply; 18+ messages in thread
From: David Laight @ 2013-12-10 13:01 UTC (permalink / raw)
To: Yang Yingliang, davem, netdev
> #define SKIP_NONLOCAL(skb) \
> +({ \
> if (unlikely(skb->sk == NULL)) { \
> *err = -1; \
> return; \
> - }
> + } \
> +})
That one should be lined up against the fence and shot :-)
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis
2013-12-10 13:01 ` David Laight
@ 2013-12-11 2:16 ` Yang Yingliang
0 siblings, 0 replies; 18+ messages in thread
From: Yang Yingliang @ 2013-12-11 2:16 UTC (permalink / raw)
To: David Laight; +Cc: davem, netdev
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; \
} \
})
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis
2013-12-10 12:55 ` [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis Yang Yingliang
2013-12-10 13:01 ` David Laight
@ 2013-12-11 3:49 ` David Miller
2013-12-11 7:17 ` [PATCH net-next] net_sched: expand control flow of macro SKIP_NONLOCAL Yang Yingliang
2013-12-11 10:20 ` [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis David Laight
1 sibling, 2 replies; 18+ messages in thread
From: David Miller @ 2013-12-11 3:49 UTC (permalink / raw)
To: yangyingliang; +Cc: netdev
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;
}
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH net-next] net_sched: expand control flow of macro SKIP_NONLOCAL
2013-12-11 3:49 ` David Miller
@ 2013-12-11 7:17 ` Yang Yingliang
2013-12-11 17:29 ` David Miller
2013-12-11 10:20 ` [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis David Laight
1 sibling, 1 reply; 18+ messages in thread
From: Yang Yingliang @ 2013-12-11 7:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Yang Yingliang <yangyingliang@huawei.com>
SKIP_NONLOCAL hides the control flow. The control flow should be
inlined and expanded explicitly in code so that someone who reads
it can tell the control flow can be changed by the statement.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
net/sched/em_meta.c | 157 ++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 122 insertions(+), 35 deletions(-)
diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
index e5cef956..382519a 100644
--- a/net/sched/em_meta.c
+++ b/net/sched/em_meta.c
@@ -271,40 +271,52 @@ META_COLLECTOR(int_rtiif)
* Socket Attributes
**************************************************************************/
-#define SKIP_NONLOCAL(skb) \
- if (unlikely(skb->sk == NULL)) { \
- *err = -1; \
- return; \
- }
+#define skip_nonlocal(skb) \
+ (unlikely(skb->sk == NULL))
META_COLLECTOR(int_sk_family)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_family;
}
META_COLLECTOR(int_sk_state)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_state;
}
META_COLLECTOR(int_sk_reuse)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_reuse;
}
META_COLLECTOR(int_sk_bound_if)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
/* No error if bound_dev_if is 0, legal userspace check */
dst->value = skb->sk->sk_bound_dev_if;
}
META_COLLECTOR(var_sk_bound_if)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
if (skb->sk->sk_bound_dev_if == 0) {
dst->value = (unsigned long) "any";
@@ -322,151 +334,226 @@ META_COLLECTOR(var_sk_bound_if)
META_COLLECTOR(int_sk_refcnt)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = atomic_read(&skb->sk->sk_refcnt);
}
META_COLLECTOR(int_sk_rcvbuf)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_rcvbuf;
}
META_COLLECTOR(int_sk_shutdown)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_shutdown;
}
META_COLLECTOR(int_sk_proto)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_protocol;
}
META_COLLECTOR(int_sk_type)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_type;
}
META_COLLECTOR(int_sk_rmem_alloc)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = sk_rmem_alloc_get(skb->sk);
}
META_COLLECTOR(int_sk_wmem_alloc)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = sk_wmem_alloc_get(skb->sk);
}
META_COLLECTOR(int_sk_omem_alloc)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = atomic_read(&skb->sk->sk_omem_alloc);
}
META_COLLECTOR(int_sk_rcv_qlen)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_receive_queue.qlen;
}
META_COLLECTOR(int_sk_snd_qlen)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_write_queue.qlen;
}
META_COLLECTOR(int_sk_wmem_queued)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_wmem_queued;
}
META_COLLECTOR(int_sk_fwd_alloc)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_forward_alloc;
}
META_COLLECTOR(int_sk_sndbuf)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_sndbuf;
}
META_COLLECTOR(int_sk_alloc)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = (__force int) skb->sk->sk_allocation;
}
META_COLLECTOR(int_sk_hash)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_hash;
}
META_COLLECTOR(int_sk_lingertime)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_lingertime / HZ;
}
META_COLLECTOR(int_sk_err_qlen)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_error_queue.qlen;
}
META_COLLECTOR(int_sk_ack_bl)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_ack_backlog;
}
META_COLLECTOR(int_sk_max_ack_bl)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_max_ack_backlog;
}
META_COLLECTOR(int_sk_prio)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_priority;
}
META_COLLECTOR(int_sk_rcvlowat)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_rcvlowat;
}
META_COLLECTOR(int_sk_rcvtimeo)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_rcvtimeo / HZ;
}
META_COLLECTOR(int_sk_sndtimeo)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_sndtimeo / HZ;
}
META_COLLECTOR(int_sk_sendmsg_off)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_frag.offset;
}
META_COLLECTOR(int_sk_write_pend)
{
- SKIP_NONLOCAL(skb);
+ if (skip_nonlocal(skb)) {
+ *err = -1;
+ return;
+ }
dst->value = skb->sk->sk_write_pending;
}
-- 1.8.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* RE: [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis
2013-12-11 3:49 ` David Miller
2013-12-11 7:17 ` [PATCH net-next] net_sched: expand control flow of macro SKIP_NONLOCAL Yang Yingliang
@ 2013-12-11 10:20 ` David Laight
1 sibling, 0 replies; 18+ messages in thread
From: David Laight @ 2013-12-11 10:20 UTC (permalink / raw)
To: David Miller, yangyingliang; +Cc: netdev
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread