[PATCH] IB/core: fix duplicated code for different branches

[PATCH] IB/core: fix duplicated code for different branches

[Gustavo A. R. Silva]

Refactor code to avoid identical code for different branches.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/infiniband/core/cm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 2b4d613..b46262f 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -590,8 +590,6 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
 			link = &(*link)->rb_right;
 		else if (be64_lt(service_id, cur_cm_id_priv->id.service_id))
 			link = &(*link)->rb_left;
-		else if (be64_gt(service_id, cur_cm_id_priv->id.service_id))
-			link = &(*link)->rb_right;
 		else
 			link = &(*link)->rb_right;
 	}
@@ -619,8 +617,6 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device,
 			node = node->rb_right;
 		else if (be64_lt(service_id, cm_id_priv->id.service_id))
 			node = node->rb_left;
-		else if (be64_gt(service_id, cm_id_priv->id.service_id))
-			node = node->rb_right;
 		else
 			node = node->rb_right;
 	}
-- 
2.5.0

Re: [PATCH] IB/core: fix duplicated code for different branches

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Sat, Aug 12, 2017 at 07:52:35PM -0500, Gustavo A. R. Silva wrote:
> Refactor code to avoid identical code for different branches.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
> ---
>  drivers/infiniband/core/cm.c | 4 ----
>  1 file changed, 4 deletions(-)

I see that you used the same commit message and title for many patches,
it will be better to have description on what exactly you are removing
and why there is no bug in that code.


>
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index 2b4d613..b46262f 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -590,8 +590,6 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
>  			link = &(*link)->rb_right;
>  		else if (be64_lt(service_id, cur_cm_id_priv->id.service_id))
>  			link = &(*link)->rb_left;
> -		else if (be64_gt(service_id, cur_cm_id_priv->id.service_id))
> -			link = &(*link)->rb_right;
>  		else
>  			link = &(*link)->rb_right;
>  	}
> @@ -619,8 +617,6 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device,
>  			node = node->rb_right;
>  		else if (be64_lt(service_id, cm_id_priv->id.service_id))
>  			node = node->rb_left;
> -		else if (be64_gt(service_id, cm_id_priv->id.service_id))
> -			node = node->rb_right;

You should remove be64_gt() too.

>  		else
>  			node = node->rb_right;
>  	}
> --
> 2.5.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/core: fix duplicated code for different branches

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]

On Sat, Aug 12, 2017 at 07:52:35PM -0500, Gustavo A. R. Silva wrote:
> Refactor code to avoid identical code for different branches.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/infiniband/core/cm.c | 4 ----
>  1 file changed, 4 deletions(-)

I see that you used the same commit message and title for many patches,
it will be better to have description on what exactly you are removing
and why there is no bug in that code.


>
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index 2b4d613..b46262f 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -590,8 +590,6 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
>  			link = &(*link)->rb_right;
>  		else if (be64_lt(service_id, cur_cm_id_priv->id.service_id))
>  			link = &(*link)->rb_left;
> -		else if (be64_gt(service_id, cur_cm_id_priv->id.service_id))
> -			link = &(*link)->rb_right;
>  		else
>  			link = &(*link)->rb_right;
>  	}
> @@ -619,8 +617,6 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device,
>  			node = node->rb_right;
>  		else if (be64_lt(service_id, cm_id_priv->id.service_id))
>  			node = node->rb_left;
> -		else if (be64_gt(service_id, cm_id_priv->id.service_id))
> -			node = node->rb_right;

You should remove be64_gt() too.

>  		else
>  			node = node->rb_right;
>  	}
> --
> 2.5.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/core: fix duplicated code for different branches

[Gustavo A. R. Silva]

Hi Leon,

On 08/13/2017 01:31 AM, Leon Romanovsky wrote:
> On Sat, Aug 12, 2017 at 07:52:35PM -0500, Gustavo A. R. Silva wrote:
>> Refactor code to avoid identical code for different branches.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/infiniband/core/cm.c | 4 ----
>>  1 file changed, 4 deletions(-)
>
> I see that you used the same commit message and title for many patches,
> it will be better to have description on what exactly you are removing
> and why there is no bug in that code.
>

Thank you for the suggestion.

I will add a note saying that it is up to the maintainer to verify 
whether the code is applicable or not.

The intention of the patch is to point out a duplication of code found 
by a Coccinelle script. I'm also working on similar issues reported by 
Coverity. It was tested only by compilation as I don't have the proper 
hardware to test it on.

>
>>
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index 2b4d613..b46262f 100644
>> --- a/drivers/infiniband/core/cm.c
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -590,8 +590,6 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
>>  			link = &(*link)->rb_right;
>>  		else if (be64_lt(service_id, cur_cm_id_priv->id.service_id))
>>  			link = &(*link)->rb_left;
>> -		else if (be64_gt(service_id, cur_cm_id_priv->id.service_id))
>> -			link = &(*link)->rb_right;
>>  		else
>>  			link = &(*link)->rb_right;
>>  	}
>> @@ -619,8 +617,6 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device,
>>  			node = node->rb_right;
>>  		else if (be64_lt(service_id, cm_id_priv->id.service_id))
>>  			node = node->rb_left;
>> -		else if (be64_gt(service_id, cm_id_priv->id.service_id))
>> -			node = node->rb_right;
>
> You should remove be64_gt() too.
>

Thank you for pointing it out.

>>  		else
>>  			node = node->rb_right;
>>  	}
>> --
>> 2.5.0
>>

-- 
Gustavo A. R. Silva