Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin Ian King]

On 18/02/18 16:31, Joe Perches wrote:
> On Sun, 2018-02-18 at 16:58 +0200, Andy Shevchenko wrote:
>> On Fri, Feb 16, 2018 at 6:53 PM, Colin Ian King
>>  wrote:
>>> On 16/02/18 16:51, Andy Shevchenko wrote:
 On Thu, Feb 15, 2018 at 9:42 PM, Colin King  
 wrote:
> +   filter->f.mask.tcp_spec.dst_ip[i] |=
> 
> cpu_to_be32(0x);

 Can it be one line then?
>>>
>>> I re-adjusted the text because checkpatch was complaining.

> +   filter->f.mask.tcp_spec.src_ip[i] |=
> 
> cpu_to_be32(0x);
>>
>> For the rest OK, but for the above two how much over 80 it went if
>> would be one line?
>> If it 2-3 characters, consider to make it one line. It would increase
>> readability.
> 
> Another possibility would be to use temporaries for
>   filter->f.mask.tcp_spec
> and
>   filter->f.data.tcp_spec
> as both are used ~10 times in the function

That's a good idea. I'll fix this up tomorrow when I get back to work
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin Ian King]

On 18/02/18 16:31, Joe Perches wrote:
> On Sun, 2018-02-18 at 16:58 +0200, Andy Shevchenko wrote:
>> On Fri, Feb 16, 2018 at 6:53 PM, Colin Ian King
>>  wrote:
>>> On 16/02/18 16:51, Andy Shevchenko wrote:
 On Thu, Feb 15, 2018 at 9:42 PM, Colin King  
 wrote:
> +   filter->f.mask.tcp_spec.dst_ip[i] |=
> 
> cpu_to_be32(0x);

 Can it be one line then?
>>>
>>> I re-adjusted the text because checkpatch was complaining.

> +   filter->f.mask.tcp_spec.src_ip[i] |=
> 
> cpu_to_be32(0x);
>>
>> For the rest OK, but for the above two how much over 80 it went if
>> would be one line?
>> If it 2-3 characters, consider to make it one line. It would increase
>> readability.
> 
> Another possibility would be to use temporaries for
>   filter->f.mask.tcp_spec
> and
>   filter->f.data.tcp_spec
> as both are used ~10 times in the function

That's a good idea. I'll fix this up tomorrow when I get back to work
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Joe Perches]

On Sun, 2018-02-18 at 16:58 +0200, Andy Shevchenko wrote:
> On Fri, Feb 16, 2018 at 6:53 PM, Colin Ian King
>  wrote:
> > On 16/02/18 16:51, Andy Shevchenko wrote:
> > > On Thu, Feb 15, 2018 at 9:42 PM, Colin King  
> > > wrote:
> > > > +   filter->f.mask.tcp_spec.dst_ip[i] |=
> > > > 
> > > > cpu_to_be32(0x);
> > > 
> > > Can it be one line then?
> > 
> > I re-adjusted the text because checkpatch was complaining.
> > > 
> > > > +   filter->f.mask.tcp_spec.src_ip[i] |=
> > > > 
> > > > cpu_to_be32(0x);
> 
> For the rest OK, but for the above two how much over 80 it went if
> would be one line?
> If it 2-3 characters, consider to make it one line. It would increase
> readability.

Another possibility would be to use temporaries for
filter->f.mask.tcp_spec
and
filter->f.data.tcp_spec
as both are used ~10 times in the function

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Joe Perches]

On Sun, 2018-02-18 at 16:58 +0200, Andy Shevchenko wrote:
> On Fri, Feb 16, 2018 at 6:53 PM, Colin Ian King
>  wrote:
> > On 16/02/18 16:51, Andy Shevchenko wrote:
> > > On Thu, Feb 15, 2018 at 9:42 PM, Colin King  
> > > wrote:
> > > > +   filter->f.mask.tcp_spec.dst_ip[i] |=
> > > > 
> > > > cpu_to_be32(0x);
> > > 
> > > Can it be one line then?
> > 
> > I re-adjusted the text because checkpatch was complaining.
> > > 
> > > > +   filter->f.mask.tcp_spec.src_ip[i] |=
> > > > 
> > > > cpu_to_be32(0x);
> 
> For the rest OK, but for the above two how much over 80 it went if
> would be one line?
> If it 2-3 characters, consider to make it one line. It would increase
> readability.

Another possibility would be to use temporaries for
filter->f.mask.tcp_spec
and
filter->f.data.tcp_spec
as both are used ~10 times in the function

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Andy Shevchenko]

On Fri, Feb 16, 2018 at 6:53 PM, Colin Ian King
 wrote:
> On 16/02/18 16:51, Andy Shevchenko wrote:
>> On Thu, Feb 15, 2018 at 9:42 PM, Colin King  wrote:

>>> +   filter->f.mask.tcp_spec.dst_ip[i] |=
>>> 
>>> cpu_to_be32(0x);
>>
>> Can it be one line then?
>
> I re-adjusted the text because checkpatch was complaining.

>>
>>> +   filter->f.mask.tcp_spec.src_ip[i] |=
>>> 
>>> cpu_to_be32(0x);

For the rest OK, but for the above two how much over 80 it went if
would be one line?
If it 2-3 characters, consider to make it one line. It would increase
readability.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Andy Shevchenko]

On Fri, Feb 16, 2018 at 6:53 PM, Colin Ian King
 wrote:
> On 16/02/18 16:51, Andy Shevchenko wrote:
>> On Thu, Feb 15, 2018 at 9:42 PM, Colin King  wrote:

>>> +   filter->f.mask.tcp_spec.dst_ip[i] |=
>>> 
>>> cpu_to_be32(0x);
>>
>> Can it be one line then?
>
> I re-adjusted the text because checkpatch was complaining.

>>
>>> +   filter->f.mask.tcp_spec.src_ip[i] |=
>>> 
>>> cpu_to_be32(0x);

For the rest OK, but for the above two how much over 80 it went if
would be one line?
If it 2-3 characters, consider to make it one line. It would increase
readability.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin Ian King]

On 16/02/18 16:51, Andy Shevchenko wrote:
> On Thu, Feb 15, 2018 at 9:42 PM, Colin King  wrote:
>> From: Colin Ian King 
>>
>> The checks to see if key->dst.s6_addr and key->src.s6_addr are null
>> pointers are redundant because these are constant size arrays and
>> so the checks always return true.  Fix this by removing the redundant
>> checks.
> 
>> +   for (i = 0; i < 4; i++)
> 
>> +   filter->f.mask.tcp_spec.dst_ip[i] |=
>> 
>> cpu_to_be32(0x);
> 
> Can it be one line then?

I re-adjusted the text because checkpatch was complaining.

> 
>> +   memcpy(>f.data.tcp_spec.dst_ip,
>> +  >dst.s6_addr32,
> 
> Ditto.
> 
>> +  sizeof(filter->f.data.tcp_spec.dst_ip));
>> +
>> +   for (i = 0; i < 4; i++)
> 
>> +   filter->f.mask.tcp_spec.src_ip[i] |=
>> 
>> cpu_to_be32(0x);
> 
> Ditto.
> 
>> +   memcpy(>f.data.tcp_spec.src_ip,
>> +  >src.s6_addr32,
> 
> Ditto.
> 
>> +  sizeof(filter->f.data.tcp_spec.src_ip));
> 

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin Ian King]

On 16/02/18 16:51, Andy Shevchenko wrote:
> On Thu, Feb 15, 2018 at 9:42 PM, Colin King  wrote:
>> From: Colin Ian King 
>>
>> The checks to see if key->dst.s6_addr and key->src.s6_addr are null
>> pointers are redundant because these are constant size arrays and
>> so the checks always return true.  Fix this by removing the redundant
>> checks.
> 
>> +   for (i = 0; i < 4; i++)
> 
>> +   filter->f.mask.tcp_spec.dst_ip[i] |=
>> 
>> cpu_to_be32(0x);
> 
> Can it be one line then?

I re-adjusted the text because checkpatch was complaining.

> 
>> +   memcpy(>f.data.tcp_spec.dst_ip,
>> +  >dst.s6_addr32,
> 
> Ditto.
> 
>> +  sizeof(filter->f.data.tcp_spec.dst_ip));
>> +
>> +   for (i = 0; i < 4; i++)
> 
>> +   filter->f.mask.tcp_spec.src_ip[i] |=
>> 
>> cpu_to_be32(0x);
> 
> Ditto.
> 
>> +   memcpy(>f.data.tcp_spec.src_ip,
>> +  >src.s6_addr32,
> 
> Ditto.
> 
>> +  sizeof(filter->f.data.tcp_spec.src_ip));
> 

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Andy Shevchenko]

On Thu, Feb 15, 2018 at 9:42 PM, Colin King  wrote:
> From: Colin Ian King 
>
> The checks to see if key->dst.s6_addr and key->src.s6_addr are null
> pointers are redundant because these are constant size arrays and
> so the checks always return true.  Fix this by removing the redundant
> checks.

> +   for (i = 0; i < 4; i++)

> +   filter->f.mask.tcp_spec.dst_ip[i] |=
> 
> cpu_to_be32(0x);

Can it be one line then?

> +   memcpy(>f.data.tcp_spec.dst_ip,
> +  >dst.s6_addr32,

Ditto.

> +  sizeof(filter->f.data.tcp_spec.dst_ip));
> +
> +   for (i = 0; i < 4; i++)

> +   filter->f.mask.tcp_spec.src_ip[i] |=
> 
> cpu_to_be32(0x);

Ditto.

> +   memcpy(>f.data.tcp_spec.src_ip,
> +  >src.s6_addr32,

Ditto.

> +  sizeof(filter->f.data.tcp_spec.src_ip));

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Andy Shevchenko]

On Thu, Feb 15, 2018 at 9:42 PM, Colin King  wrote:
> From: Colin Ian King 
>
> The checks to see if key->dst.s6_addr and key->src.s6_addr are null
> pointers are redundant because these are constant size arrays and
> so the checks always return true.  Fix this by removing the redundant
> checks.

> +   for (i = 0; i < 4; i++)

> +   filter->f.mask.tcp_spec.dst_ip[i] |=
> 
> cpu_to_be32(0x);

Can it be one line then?

> +   memcpy(>f.data.tcp_spec.dst_ip,
> +  >dst.s6_addr32,

Ditto.

> +  sizeof(filter->f.data.tcp_spec.dst_ip));
> +
> +   for (i = 0; i < 4; i++)

> +   filter->f.mask.tcp_spec.src_ip[i] |=
> 
> cpu_to_be32(0x);

Ditto.

> +   memcpy(>f.data.tcp_spec.src_ip,
> +  >src.s6_addr32,

Ditto.

> +  sizeof(filter->f.data.tcp_spec.src_ip));

-- 
With Best Regards,
Andy Shevchenko

[PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin King]

From: Colin Ian King 

The checks to see if key->dst.s6_addr and key->src.s6_addr are null
pointers are redundant because these are constant size arrays and
so the checks always return true.  Fix this by removing the redundant
checks.

Detected by CoverityScan, CID#1465279 ("Array compared to 0")

Signed-off-by: Colin Ian King 
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 25 +++--
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c 
b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 4955ce3ab6a2..63e4169828ea 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -2710,22 +2710,19 @@ static int i40evf_parse_cls_flower(struct 
i40evf_adapter *adapter,
if (!ipv6_addr_any(>dst) || !ipv6_addr_any(>src))
field_flags |= I40EVF_CLOUD_FIELD_IIP;
 
-   if (key->dst.s6_addr) {
-   for (i = 0; i < 4; i++)
-   filter->f.mask.tcp_spec.dst_ip[i] |=
+   for (i = 0; i < 4; i++)
+   filter->f.mask.tcp_spec.dst_ip[i] |=
cpu_to_be32(0x);
-   memcpy(>f.data.tcp_spec.dst_ip,
-  >dst.s6_addr32,
-  sizeof(filter->f.data.tcp_spec.dst_ip));
-   }
-   if (key->src.s6_addr) {
-   for (i = 0; i < 4; i++)
-   filter->f.mask.tcp_spec.src_ip[i] |=
+   memcpy(>f.data.tcp_spec.dst_ip,
+  >dst.s6_addr32,
+  sizeof(filter->f.data.tcp_spec.dst_ip));
+
+   for (i = 0; i < 4; i++)
+   filter->f.mask.tcp_spec.src_ip[i] |=
cpu_to_be32(0x);
-   memcpy(>f.data.tcp_spec.src_ip,
-  >src.s6_addr32,
-  sizeof(filter->f.data.tcp_spec.src_ip));
-   }
+   memcpy(>f.data.tcp_spec.src_ip,
+  >src.s6_addr32,
+  sizeof(filter->f.data.tcp_spec.src_ip));
}
if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_PORTS)) {
struct flow_dissector_key_ports *key =
-- 
2.15.1

[PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin King]

From: Colin Ian King 

The checks to see if key->dst.s6_addr and key->src.s6_addr are null
pointers are redundant because these are constant size arrays and
so the checks always return true.  Fix this by removing the redundant
checks.

Detected by CoverityScan, CID#1465279 ("Array compared to 0")

Signed-off-by: Colin Ian King 
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 25 +++--
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c 
b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 4955ce3ab6a2..63e4169828ea 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -2710,22 +2710,19 @@ static int i40evf_parse_cls_flower(struct 
i40evf_adapter *adapter,
if (!ipv6_addr_any(>dst) || !ipv6_addr_any(>src))
field_flags |= I40EVF_CLOUD_FIELD_IIP;
 
-   if (key->dst.s6_addr) {
-   for (i = 0; i < 4; i++)
-   filter->f.mask.tcp_spec.dst_ip[i] |=
+   for (i = 0; i < 4; i++)
+   filter->f.mask.tcp_spec.dst_ip[i] |=
cpu_to_be32(0x);
-   memcpy(>f.data.tcp_spec.dst_ip,
-  >dst.s6_addr32,
-  sizeof(filter->f.data.tcp_spec.dst_ip));
-   }
-   if (key->src.s6_addr) {
-   for (i = 0; i < 4; i++)
-   filter->f.mask.tcp_spec.src_ip[i] |=
+   memcpy(>f.data.tcp_spec.dst_ip,
+  >dst.s6_addr32,
+  sizeof(filter->f.data.tcp_spec.dst_ip));
+
+   for (i = 0; i < 4; i++)
+   filter->f.mask.tcp_spec.src_ip[i] |=
cpu_to_be32(0x);
-   memcpy(>f.data.tcp_spec.src_ip,
-  >src.s6_addr32,
-  sizeof(filter->f.data.tcp_spec.src_ip));
-   }
+   memcpy(>f.data.tcp_spec.src_ip,
+  >src.s6_addr32,
+  sizeof(filter->f.data.tcp_spec.src_ip));
}
if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_PORTS)) {
struct flow_dissector_key_ports *key =
-- 
2.15.1