[PATCH 0/3] bridge: netfilter: checkpatch fixes

[PATCH 0/3] bridge: netfilter: checkpatch fixes

[tcharding]

From: Tobin C Harding <me@tobin.cc>

checkpatch produces 32 'checks'. This patch set amends them all.

Patch one is white space only.
Patch two changes data types e.g uint8_t -> u8.
Patch three amends comparison to null.

Tobin C Harding (3):
  bridge: netfilter: checkpatch whitespace fixes
  bridge: netfilter: checkpatch data type fixes
  bridge: netfilter: checkpatch null comparison fixes

 net/bridge/netfilter/ebt_stp.c | 68 +++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

-- 
2.8.2

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

[tcharding]

From: Tobin C Harding <me@tobin.cc>

checkpatch produces various white space 'checks'.

This patch amends them.

Signed-off-by: Tobin C Harding <me@tobin.cc>
---
This is my second linux kernel patch. Unsure if I was meant to cc multiple mailing lists?

thanks

 net/bridge/netfilter/ebt_stp.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
index 6b731e1..26a0859 100644
--- 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))
 			return false;
 	}
 	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];
 		if (FWINV(verdict != 0, EBT_STP_ROOTADDR))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_ROOTCOST) {
 		v32 = NR32(stpc->root_cost);
 		if (FWINV(v32 < c->root_costl ||
-		    v32 > c->root_costu, EBT_STP_ROOTCOST))
+			  v32 > c->root_costu, EBT_STP_ROOTCOST))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_SENDERPRIO) {
 		v16 = NR16(stpc->sender);
 		if (FWINV(v16 < c->sender_priol ||
-		    v16 > c->sender_priou, EBT_STP_SENDERPRIO))
+			  v16 > c->sender_priou, EBT_STP_SENDERPRIO))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_SENDERADDR) {
 		verdict = 0;
 		for (i = 0; i < 6; i++)
-			verdict |= (stpc->sender[2+i] ^ c->sender_addr[i]) &
-				   c->sender_addrmsk[i];
+			verdict |= (stpc->sender[2 + i] ^ c->sender_addr[i]) &
+				c->sender_addrmsk[i];
 		if (FWINV(verdict != 0, EBT_STP_SENDERADDR))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_PORT) {
 		v16 = NR16(stpc->port);
 		if (FWINV(v16 < c->portl ||
-		    v16 > c->portu, EBT_STP_PORT))
+			  v16 > c->portu, EBT_STP_PORT))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_MSGAGE) {
 		v16 = NR16(stpc->msg_age);
 		if (FWINV(v16 < c->msg_agel ||
-		    v16 > c->msg_ageu, EBT_STP_MSGAGE))
+			  v16 > c->msg_ageu, EBT_STP_MSGAGE))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_MAXAGE) {
 		v16 = NR16(stpc->max_age);
 		if (FWINV(v16 < c->max_agel ||
-		    v16 > c->max_ageu, EBT_STP_MAXAGE))
+			  v16 > c->max_ageu, EBT_STP_MAXAGE))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_HELLOTIME) {
 		v16 = NR16(stpc->hello_time);
 		if (FWINV(v16 < c->hello_timel ||
-		    v16 > c->hello_timeu, EBT_STP_HELLOTIME))
+			  v16 > c->hello_timeu, EBT_STP_HELLOTIME))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_FWDD) {
 		v16 = NR16(stpc->forward_delay);
 		if (FWINV(v16 < c->forward_delayl ||
-		    v16 > c->forward_delayu, EBT_STP_FWDD))
+			  v16 > c->forward_delayu, EBT_STP_FWDD))
 			return false;
 	}
 	return true;
-- 
2.8.2

[PATCH 2/3] bridge: netfilter: checkpatch data type fixes

[tcharding]

From: Tobin C Harding <me@tobin.cc>

checkpatch produces data type 'checks'.

This patch amends them by changing, for example:
uint8_t -> u8

Signed-off-by: Tobin C Harding <me@tobin.cc>
---
 net/bridge/netfilter/ebt_stp.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
index 26a0859..9df943f 100644
--- a/net/bridge/netfilter/ebt_stp.c
+++ b/net/bridge/netfilter/ebt_stp.c
@@ -17,24 +17,24 @@
 #define BPDU_TYPE_TCN 0x80
 
 struct stp_header {
-	uint8_t dsap;
-	uint8_t ssap;
-	uint8_t ctrl;
-	uint8_t pid;
-	uint8_t vers;
-	uint8_t type;
+	u8 dsap;
+	u8 ssap;
+	u8 ctrl;
+	u8 pid;
+	u8 vers;
+	u8 type;
 };
 
 struct stp_config_pdu {
-	uint8_t flags;
-	uint8_t root[8];
-	uint8_t root_cost[4];
-	uint8_t sender[8];
-	uint8_t port[2];
-	uint8_t msg_age[2];
-	uint8_t max_age[2];
-	uint8_t hello_time[2];
-	uint8_t forward_delay[2];
+	u8 flags;
+	u8 root[8];
+	u8 root_cost[4];
+	u8 sender[8];
+	u8 port[2];
+	u8 msg_age[2];
+	u8 max_age[2];
+	u8 hello_time[2];
+	u8 forward_delay[2];
 };
 
 #define NR16(p) (p[0] << 8 | p[1])
@@ -44,8 +44,8 @@ static bool ebt_filter_config(const struct ebt_stp_info *info,
 			      const struct stp_config_pdu *stpc)
 {
 	const struct ebt_stp_config_info *c;
-	uint16_t v16;
-	uint32_t v32;
+	u16 v16;
+	u32 v32;
 	int verdict, i;
 
 	c = &info->config;
@@ -125,7 +125,7 @@ ebt_stp_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	const struct ebt_stp_info *info = par->matchinfo;
 	const struct stp_header *sp;
 	struct stp_header _stph;
-	const uint8_t header[6] = {0x42, 0x42, 0x03, 0x00, 0x00, 0x00};
+	const u8 header[6] = {0x42, 0x42, 0x03, 0x00, 0x00, 0x00};
 
 	sp = skb_header_pointer(skb, 0, sizeof(_stph), &_stph);
 	if (sp == NULL)
@@ -156,8 +156,8 @@ ebt_stp_mt(const struct sk_buff *skb, struct xt_action_param *par)
 static int ebt_stp_mt_check(const struct xt_mtchk_param *par)
 {
 	const struct ebt_stp_info *info = par->matchinfo;
-	const uint8_t bridge_ula[6] = {0x01, 0x80, 0xc2, 0x00, 0x00, 0x00};
-	const uint8_t msk[6] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+	const u8 bridge_ula[6] = {0x01, 0x80, 0xc2, 0x00, 0x00, 0x00};
+	const u8 msk[6] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
 	const struct ebt_entry *e = par->entryinfo;
 
 	if (info->bitmask & ~EBT_STP_MASK || info->invflags & ~EBT_STP_MASK ||
-- 
2.8.2

[PATCH 3/3] bridge: netfilter: checkpatch null comparison fixes

[tcharding]

From: Tobin C Harding <me@tobin.cc>

checkpatch produces comparison to null 'checks'.

This patch amends them.

Signed-off-by: Tobin C Harding <me@tobin.cc>
---
 net/bridge/netfilter/ebt_stp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
index 9df943f..677904f 100644
--- a/net/bridge/netfilter/ebt_stp.c
+++ b/net/bridge/netfilter/ebt_stp.c
@@ -128,7 +128,7 @@ ebt_stp_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	const u8 header[6] = {0x42, 0x42, 0x03, 0x00, 0x00, 0x00};
 
 	sp = skb_header_pointer(skb, 0, sizeof(_stph), &_stph);
-	if (sp == NULL)
+	if (!sp)
 		return false;
 
 	/* The stp code only considers these */
@@ -146,7 +146,7 @@ ebt_stp_mt(const struct sk_buff *skb, struct xt_action_param *par)
 
 		st = skb_header_pointer(skb, sizeof(_stph),
 					sizeof(_stpc), &_stpc);
-		if (st == NULL)
+		if (!st)
 			return false;
 		return ebt_filter_config(info, st);
 	}
-- 
2.8.2

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

[Pablo Neira Ayuso]

Hi,

On Tue, May 10, 2016 at 11:26:56AM +1000, tcharding wrote:
> From: Tobin C Harding <me@tobin.cc>
> 
> checkpatch produces various white space 'checks'.
> 
> This patch amends them.
> 
> Signed-off-by: Tobin C Harding <me@tobin.cc>
> ---
> This is my second linux kernel patch. Unsure if I was meant to cc multiple mailing lists?
> 
> thanks
> 
>  net/bridge/netfilter/ebt_stp.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
> index 6b731e1..26a0859 100644
> --- 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.

Re: [PATCH 2/3] bridge: netfilter: checkpatch data type fixes

[Pablo Neira Ayuso]

On Tue, May 10, 2016 at 11:26:57AM +1000, tcharding wrote:
> From: Tobin C Harding <me@tobin.cc>
> 
> checkpatch produces data type 'checks'.
> 
> This patch amends them by changing, for example:
> uint8_t -> u8

This looks good. Applied, thanks.

Re: [PATCH 3/3] bridge: netfilter: checkpatch null comparison fixes

[Pablo Neira Ayuso]

On Tue, May 10, 2016 at 11:26:58AM +1000, tcharding wrote:
> From: Tobin C Harding <me@tobin.cc>
> 
> checkpatch produces comparison to null 'checks'.
> 
> This patch amends them.

We have quite a lot of these in the netfilter tree, so I'd rather
start using prefered coding style from now on rather than getting a
large patch to convert this. This just makes harder to pass back
patches to -stable branches as the process becomes not so immediate.

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

[Joe Perches]

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.

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

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

[Pablo Neira Ayuso]

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.

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

[Joe Perches]

On Tue, 2016-06-07 at 19:34 +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote:
> > 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.

"2+i" or "2 + i", either is OK.
Multiple line statement alignment doesn't
matter much.

I think either is fine and both are "don't care, don't need"
to change from one to another to satisfy some silly whitespace
overlord brainless script.

Perhaps it's better to add a function for this though.

Something like:
---
 net/bridge/netfilter/ebt_stp.c | 60 +++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
index 6b731e1..4096fac 100644
--- a/net/bridge/netfilter/ebt_stp.c
+++ b/net/bridge/netfilter/ebt_stp.c
@@ -40,13 +40,24 @@ struct stp_config_pdu {
 #define NR16(p) (p[0] << 8 | p[1])
 #define NR32(p) ((p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3])
 
+static bool ebt_test_addr(const uint8_t *root, const char *addr,
+			  const char *mask)
+{
+	bool rtn = false;
+	int i;
+
+	for (i = 0; i < ETH_ALEN; i++)
+		rtn |= (root[2 + i] ^ addr[i]) & mask[i];
+
+	return rtn;
+}
+
 static bool ebt_filter_config(const struct ebt_stp_info *info,
 			      const struct stp_config_pdu *stpc)
 {
 	const struct ebt_stp_config_info *c;
 	uint16_t v16;
 	uint32_t v32;
-	int verdict, i;
 
 	c = &info->config;
 	if ((info->bitmask & EBT_STP_FLAGS) &&
@@ -54,66 +65,61 @@ static bool ebt_filter_config(const struct ebt_stp_info *info,
 		return false;
 	if (info->bitmask & EBT_STP_ROOTPRIO) {
 		v16 = NR16(stpc->root);
-		if (FWINV(v16 < c->root_priol ||
-		    v16 > c->root_priou, EBT_STP_ROOTPRIO))
+		if (FWINV(v16 < c->root_priol || v16 > c->root_priou,
+			  EBT_STP_ROOTPRIO))
 			return false;
 	}
 	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];
-		if (FWINV(verdict != 0, EBT_STP_ROOTADDR))
+		if (FWINV(ebt_test_addr(stpc->root, c->root_addr,
+					c->root_addrmsk),
+			  EBT_STP_ROOTADDR))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_ROOTCOST) {
 		v32 = NR32(stpc->root_cost);
-		if (FWINV(v32 < c->root_costl ||
-		    v32 > c->root_costu, EBT_STP_ROOTCOST))
+		if (FWINV(v32 < c->root_costl || v32 > c->root_costu,
+			  EBT_STP_ROOTCOST))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_SENDERPRIO) {
 		v16 = NR16(stpc->sender);
-		if (FWINV(v16 < c->sender_priol ||
-		    v16 > c->sender_priou, EBT_STP_SENDERPRIO))
+		if (FWINV(v16 < c->sender_priol || v16 > c->sender_priou,
+			  EBT_STP_SENDERPRIO))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_SENDERADDR) {
-		verdict = 0;
-		for (i = 0; i < 6; i++)
-			verdict |= (stpc->sender[2+i] ^ c->sender_addr[i]) &
-				   c->sender_addrmsk[i];
-		if (FWINV(verdict != 0, EBT_STP_SENDERADDR))
+		if (FWINV(ebt_test_addr(stpc->sender, c->sender_addr,
+					c->sender_addrmsk),
+			  EBT_STP_SENDERADDR))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_PORT) {
 		v16 = NR16(stpc->port);
-		if (FWINV(v16 < c->portl ||
-		    v16 > c->portu, EBT_STP_PORT))
+		if (FWINV(v16 < c->portl || v16 > c->portu, EBT_STP_PORT))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_MSGAGE) {
 		v16 = NR16(stpc->msg_age);
-		if (FWINV(v16 < c->msg_agel ||
-		    v16 > c->msg_ageu, EBT_STP_MSGAGE))
+		if (FWINV(v16 < c->msg_agel || v16 > c->msg_ageu,
+			  EBT_STP_MSGAGE))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_MAXAGE) {
 		v16 = NR16(stpc->max_age);
-		if (FWINV(v16 < c->max_agel ||
-		    v16 > c->max_ageu, EBT_STP_MAXAGE))
+		if (FWINV(v16 < c->max_agel || v16 > c->max_ageu,
+			  EBT_STP_MAXAGE))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_HELLOTIME) {
 		v16 = NR16(stpc->hello_time);
-		if (FWINV(v16 < c->hello_timel ||
-		    v16 > c->hello_timeu, EBT_STP_HELLOTIME))
+		if (FWINV(v16 < c->hello_timel || v16 > c->hello_timeu,
+			  EBT_STP_HELLOTIME))
 			return false;
 	}
 	if (info->bitmask & EBT_STP_FWDD) {
 		v16 = NR16(stpc->forward_delay);
-		if (FWINV(v16 < c->forward_delayl ||
-		    v16 > c->forward_delayu, EBT_STP_FWDD))
+		if (FWINV(v16 < c->forward_delayl || v16 > c->forward_delayu,
+			  EBT_STP_FWDD))
 			return false;
 	}
 	return true;

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On Tue, Jun 07, 2016 at 11:02:30AM -0700, Joe Perches wrote:
> On Tue, 2016-06-07 at 19:34 +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote:
> > > 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.
> 
> "2+i" or "2 + i", either is OK.
> Multiple line statement alignment doesn't
> matter much.

Sorry, I was actually refering to:

> > +                       verdict |= (stpc->root[2 + i] ^ c->root_addr[i]) &
> > +                               c->root_addrmsk[i];
                                    ^^^

instead of:

> > -                       verdict |= (stpc->root[2+i] ^ c->root_addr[i]) &
> > -                                  c->root_addrmsk[i];
                                       ^

here.

> I think either is fine and both are "don't care, don't need"
> to change from one to another to satisfy some silly whitespace
> overlord brainless script.
> 
> Perhaps it's better to add a function for this though.

I like this function idea :).

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

[Joe Perches]

On Wed, 2016-06-08 at 13:52 +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 07, 2016 at 11:02:30AM -0700, Joe Perches wrote:
> > On Tue, 2016-06-07 at 19:34 +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote:
> > > > 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.
> > "2+i" or "2 + i", either is OK.
> > Multiple line statement alignment doesn't
> > matter much.
> Sorry, I was actually refering to:
[]

Hi again Pablo.

No worries.  I hoped the "doesn't matter much" was clear enough.

There are many different multiple line statement alignment
styles in the kernel.

Alignment to open parenthesis is one of them, and I think it's
reasonable to standardize on that.

For multiple line statements without parentheses for alignment,
I think there isn't one style that's much better than another.

I slightly prefer the original alignment above myself.

> > Perhaps it's better to add a function for this though.
> I like this function idea :).

Maybe something like this is clearer:

static bool ebt_test_addr(const uint8_t *root, const char *addr,
			  const char *mask)
{
	int i;

	for (i = 0; i < ETH_ALEN; i++) {
		if ((root[2 + i] ^ addr[i]) & mask[i])
			return true;
	}

	return false;
}

Maybe the call should add the + 2 to the first argument
instead of using + 2 in the loop.

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

[Pablo Neira Ayuso]

On Wed, Jun 08, 2016 at 09:52:30AM -0700, Joe Perches wrote:
> On Wed, 2016-06-08 at 13:52 +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jun 07, 2016 at 11:02:30AM -0700, Joe Perches wrote:
> > > On Tue, 2016-06-07 at 19:34 +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Jun 07, 2016 at 10:04:40AM -0700, Joe Perches wrote:
> > > > > 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.
> > > "2+i" or "2 + i", either is OK.
> > > Multiple line statement alignment doesn't
> > > matter much.
> > Sorry, I was actually refering to:
> []
> 
> Hi again Pablo.
> 
> No worries.  I hoped the "doesn't matter much" was clear enough.
> 
> There are many different multiple line statement alignment
> styles in the kernel.
> 
> Alignment to open parenthesis is one of them, and I think it's
> reasonable to standardize on that.
> 
> For multiple line statements without parentheses for alignment,
> I think there isn't one style that's much better than another.
> 
> I slightly prefer the original alignment above myself.

I do too. I can take Tobin's original patch and manually revert this
chunk then, ie.

-                       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];

> Maybe something like this is clearer:
> 
> static bool ebt_test_addr(const uint8_t *root, const char *addr,
> 			  const char *mask)
> {
> 	int i;
> 
> 	for (i = 0; i < ETH_ALEN; i++) {
> 		if ((root[2 + i] ^ addr[i]) & mask[i])
> 			return true;
> 	}
> 
> 	return false;
> }
> 
> Maybe the call should add the + 2 to the first argument
> instead of using + 2 in the loop.

Then you can follow up with a patch to add this function.

Just a suggestion, let me know if this is fine with you.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On Wed, Jun 08, 2016 at 07:31:21PM +0200, Pablo Neira Ayuso wrote:
> Then you can follow up with a patch to add this function.
> 
> Just a suggestion, let me know if this is fine with you.

Forget this idea.

Actually your patch from: Date: Tue, 07 Jun 2016 11:02:30 -0700

looks easier to readable than original Tobin's, so I'll wait for you
to resubmit.

Thanks.

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

[Joe Perches]

On Wed, 2016-06-08 at 19:38 +0200, Pablo Neira Ayuso wrote:
> On Wed, Jun 08, 2016 at 07:31:21PM +0200, Pablo Neira Ayuso wrote:
> > Then you can follow up with a patch to add this function.
> > 
> > Just a suggestion, let me know if this is fine with you.
> Forget this idea.
> 
> Actually your patch from: Date: Tue, 07 Jun 2016 11:02:30 -0700
> 
> looks easier to readable than original Tobin's, so I'll wait for you
> to resubmit.

Well, maybe next week after the other couple patches are in -next.

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

[Tobin Harding]

On Thu, Jun 09, 2016 at 11:00:18AM -0700, Joe Perches wrote:
> On Wed, 2016-06-08 at 19:38 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Jun 08, 2016 at 07:31:21PM +0200, Pablo Neira Ayuso wrote:
> > looks easier to readable than original Tobin's, so I'll wait for you
> > to resubmit.
> 
> Well, maybe next week after the other couple patches are in -next.
> 

Not sure if I should have been adding anything to this thread since I submitted
the original patch. I'm new to the LKML and have not been able to follow the
subtleties of conversation in this thread about who wants who to do what or if
anyone needs to do anything.

If there is any follow up that is required (or requested) of me I am more than
happy to do so. I hope I have not created unnecessary work for you fellas by
adding this whitespace patch to the set. I will be more cautious about making
changes to satisfy checkpatch.pl in future.

thanks,
Tobin.