Re: [PATCH net-next v5] net: ipv6: fix code style error and warning of ndisc.c
From: Joe Perches <joe@perches.com>
Date: 2017-05-21 16:05:11
On Sun, 2017-05-21 at 08:55 +0800, yuan linyu wrote:
quoted hunk ↗ jump to hunk
From: yuan linyu <redacted>diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
[]
quoted hunk ↗ jump to hunk
@@ -512,7 +519,8 @@ void ndisc_send_na(struct net_device *dev, const struct in6_addr *daddr, in6_ifa_put(ifp); } else { if (ipv6_dev_get_saddr(dev_net(dev), dev, daddr, - inet6_sk(dev_net(dev)->ipv6.ndisc_sk)->srcprefs, + inet6_sk(dev_net(dev)->ipv6.ndisc_sk)-> + srcprefs,
This is not a good change as it puts a single dereference on multiple lines.
quoted hunk ↗ jump to hunk
@@ -896,20 +910,19 @@ static void ndisc_recv_ns(struct sk_buff *skb) else NEIGH_CACHE_STAT_INC(&nd_tbl, rcv_probes_ucast); - /* - * update / create cache entry + /* update / create cache entry * for the source address
Some of these comments could be single line
quoted hunk ↗ jump to hunk
@@ -1003,30 +1016,31 @@ static void ndisc_recv_na(struct sk_buff *skb)
[]
ndisc_update(dev, neigh, lladdr, - msg->icmph.icmp6_solicited ? NUD_REACHABLE : NUD_STALE, - NEIGH_UPDATE_F_WEAK_OVERRIDE| - (msg->icmph.icmp6_override ? NEIGH_UPDATE_F_OVERRIDE : 0)| - NEIGH_UPDATE_F_OVERRIDE_ISROUTER| - (msg->icmph.icmp6_router ? NEIGH_UPDATE_F_ISROUTER : 0), + msg->icmph.icmp6_solicited ? + NUD_REACHABLE : NUD_STALE, + NEIGH_UPDATE_F_WEAK_OVERRIDE | + (msg->icmph.icmp6_override ? + NEIGH_UPDATE_F_OVERRIDE : 0) | + NEIGH_UPDATE_F_OVERRIDE_ISROUTER | + (msg->icmph.icmp6_router ? + NEIGH_UPDATE_F_ISROUTER : 0), NDISC_NEIGHBOUR_ADVERTISEMENT, &ndopts);
This is much more difficult to read now and could be slightly improved by a temporary for msg->icmph, removing unnecessary parentheses or using multiple line tests for the flags argument of ndisc_update instead of the slightly difficult to read uses of multiple ternaries with bitwise ORs. Something like: struct icmp6hdr *icmph = &msg->icmph; u32 flags; [...] flags = NEIGH_UPDATE_F_WEAK_OVERRIDE | NEIGH_UPDATE_F_OVERRIDE_ISROUTER; if (icmph->icmp6_override) flags |= NEIGH_UPDATE_F_OVERRIDE; if (icmph->icmp6_router) flags |= NEIGH_UPDATE_F_ISROUTER; ndisc_update(dev, neigh, lladdr, icmph->icmp6_solicited ? NUD_REACHABLE : NUD_STALE, flags, NDISC_NEIGHBOUR_ADVERTISEMENT, &ndopts); But really, why bother? Just because checkpatch bleats some message doesn't mean it _has_ to be fixed. Please strive to make the code more readable and intelligible for _humans_. Compilers don't care.