netdev - Re: [PATCH 1/3] bridge: netfilter: checkpatch whitespace fixes

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, 7 Jun 2016 19:34:17 +0200
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Joe Perches <joe@...ches.com>
Cc:	tcharding <me@...in.cc>,
	Stephen Hemminger <stephen@...workplumber.org>,
	"David S. Miller" <davem@...emloft.net>,
	netfilter-devel@...r.kernel.org, coreteam@...filter.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH 1/3] bridge: netfilter: checkpatch whitespace fixes

On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote:
> On Tue, 2016-06-07 at 17:14 +0200, Pablo Neira Ayuso wrote:
> > On Tue, May 10, 2016 at 11:26:56AM +1000, tcharding wrote:
> > > From: Tobin C Harding <me@...in.cc>
> > > This is my second linux kernel patch. Unsure if I was meant to cc multiple mailing lists?
> []
> > > diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
> []
> > > @@ -55,65 +55,65 @@ static bool ebt_filter_config(const struct ebt_stp_info *info,
> > >  	if (info->bitmask & EBT_STP_ROOTPRIO) {
> > >  		v16 = NR16(stpc->root);
> > >  		if (FWINV(v16 < c->root_priol ||
> > > -		    v16 > c->root_priou, EBT_STP_ROOTPRIO))
> > > +			  v16 > c->root_priou, EBT_STP_ROOTPRIO))
> > I don't think this coding style is right. This is a common approach
> > (to align the condition when split in several lines) in other 'net' code.
> 
> Perhaps you misread the code.

Oh right. This FWINV() got me confused.

> The alignment is changed for the 1st argument of the FWINV macro
> to be more similar to the style used in the rest of net/
> 
> But using a longer initial line would be more readable:
> 
>  		if (FWINV(v16 < c->root_priol || v16 > c->root_priou,
> 			  EBT_STP_ROOTPRIO))

I see. Thanks for clarifying all the FWINV() related changes.

One more question, is this chunk below correct from coding style point
of view?

        if (info->bitmask & EBT_STP_ROOTADDR) {
                verdict = 0;
                for (i = 0; i < 6; i++)
-                       verdict |= (stpc->root[2+i] ^ c->root_addr[i]) &
-                                  c->root_addrmsk[i];
+                       verdict |= (stpc->root[2 + i] ^ c->root_addr[i]) &
+                               c->root_addrmsk[i];

I think the previous line is fine.