netdev - Re: [PATCH net-next v2] ipvlan: Initial check-in of the IPVLAN driver.

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 for Android: free password hash cracker in your pocket

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

Message-Id: <20141118.155437.1582768084232091503.davem@davemloft.net>
Date:	Tue, 18 Nov 2014 15:54:37 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	maheshb@...gle.com
Cc:	netdev@...r.kernel.org, edumazet@...gle.com, maze@...gle.com,
	chavey@...gle.com, thockin@...gle.com, brandon.philips@...eos.com,
	xemul@...allels.com
Subject: Re: [PATCH net-next v2] ipvlan: Initial check-in of the IPVLAN
 driver.

From: Mahesh Bandewar <maheshb@...gle.com>
Date: Sun, 16 Nov 2014 22:27:14 -0800

> +/* Define IPVL_DEBUG and set the appropriate dbg_level for debugging. */
> +#ifdef	IPVL_DEBUG
> +/*
> + * 1 : non-datapath debugging
> + * 2 : Custom
> + * 3 : function enters and exists.
> + * 4 : printk in data path (be careful!)
> + */
> +#define IPVL_DBG_LEVEL 1
> +#define ipvlan_dbg(level, msg...)	do { \
> +						if (level <= IPVL_DBG_LEVEL) \
> +						printk(KERN_DEBUG msg); \
> +					} while (0)
> +#else
> +#define ipvlan_dbg(level, msg...) do { ; } while (0)
> +#endif

The day of having code use custom local debug logging facilities is long
gone, please use a standard mechanism for this rather than home cooked
reimplementations.

> +/* ---- Prototype declarations ---- */
> +/* ---- ipvlan_main.c ---- */

Everone knows these are prototype declarations by virtue of them being
functions declarations in a header file.

Anyone who wants to know where these are defined can run grep.

So these comments are superfluous, please remove them across this
entire submission.

> +	const struct in_addr *ip4_addr = iaddr;
> +	return jhash_1word(ip4_addr->s_addr, ipvlan_jhash_secret)
> +	       & IPVLAN_HASH_MASK;

When an expression spans multiple lines, lines end rather than begin
with an operator.

> +		if (is_v6 && addr->atype == IPVL_IPV6 &&
> +			ipv6_addr_equal(&addr->ip6addr, iaddr))

For multi-line if() statements, the second line and subsequent lines
must begine exactly at the first column after the openning parenthesis
of the first line.

If you are purely using TAB characters to indent, you are likely doing
it wrong.
> +		if ((is_v6 && addr->atype == IPVL_IPV6 &&
> +		     ipv6_addr_equal(&addr->ip6addr, iaddr))
> +		   || (!is_v6 && addr->atype == IPVL_IPV4 &&
> +		      addr->ip4addr.s_addr == ((struct in_addr *)iaddr)->s_addr)

Lines end, not begin, with operators.

> --- /dev/null
> +++ b/drivers/net/ipvlan/ipvlan_sysfs.c

Please do not add sysfs configuration knobs, netlink is sufficient.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html