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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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