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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 18 Nov 2014 21:27:48 -0800
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	David Miller <davem@...emloft.net>
Cc:	linux-netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Maciej Żenczykowski <maze@...gle.com>,
	Laurent Chavey <chavey@...gle.com>,
	Tim Hockin <thockin@...gle.com>, brandon.philips@...eos.com,
	xemul@...allels.com
Subject: Re: [PATCH net-next v2] ipvlan: Initial check-in of the IPVLAN driver.

On Tue, Nov 18, 2014 at 12:54 PM, David Miller <davem@...emloft.net> wrote:
>
> 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.
>
Do you mean pr_err() / pr_warn() / pr_debug() etc.? or something else?

> > +/* ---- 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.

Having it does neither add complexity nor too much code, wo why not?
May be I'm missing something! But I was think that this might provide
foundation for adding other things like config locking from master's
namespace etc. (discussed in the thread) in the future.

Thanks,
--mahesh..
--
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