netdev - Re: [PATCH net-next v5] net: ipv6: fix code style error and warning of ndisc.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1495382708.2093.10.camel@perches.com>
Date:   Sun, 21 May 2017 09:05:08 -0700
From:   Joe Perches <joe@...ches.com>
To:     yuan linyu <cugyly@....com>, netdev@...r.kernel.org
Cc:     "David S . Miller" <davem@...emloft.net>,
        yuan linyu <Linyu.Yuan@...atel-sbell.com.cn>
Subject: Re: [PATCH net-next v5] net: ipv6: fix code style error and warning
 of ndisc.c

On Sun, 2017-05-21 at 08:55 +0800, yuan linyu wrote:
> From: yuan linyu <Linyu.Yuan@...atel-sbell.com.cn>
> 
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
[]
> @@ -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.

> @@ -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

> @@ -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.