question about identical code for different branches

infiniband-hw-i40iw Applied Gustavo A. R. Silva 6 participants 9 messages May 17, 2017 – Jun 1, 2017 InfiniBand

Share & tools

Gustavo A. R. Silva

author

2017-05-17 22:06 UTC #0 lore raw

Hello everybody,

While looking into Coverity ID 1362263 I ran into the following piece of code at drivers/infiniband/hw/i40iw/i40iw_virtchnl.c:445:

445 if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
446 if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
447 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
448 else
449 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
450 return I40IW_SUCCESS;
451 }

The issue is that lines of code 447 and 449 are identical for different branches.

My question here is if one of the branches should be modified, or the entire _if_ statement replaced?

Maybe a patch like the following could be applied:

index f4d1368..48fd327 100644

drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +1 -4

@@ -443,10 +443,7 @@ enum i40iw_status_code i40iw_vchnl_recv_pf(struct i40iw_sc_dev *dev,
        if (!dev->vchnl_up)
                return I40IW_ERR_NOT_READY;
        if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
-               if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
-               else
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
+               vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
                return I40IW_SUCCESS;
        }
        for (iw_vf_idx = 0; iw_vf_idx <

I40IW_MAX_PE_ENABLED_VF_COUNT; iw_vf_idx++) {

What do you think?

I'd really appreciate any comment on this.

Thank you!
--
Gustavo A. R. Silva

Leon Romanovsky

2017-05-18 05:00 UTC #1 lore raw

Quoted 14 lines · Gustavo A. R. Silva

Hello everybody,
While looking into Coverity ID 1362263 I ran into the following piece of code at drivers/infiniband/hw/i40iw/i40iw_virtchnl.c:445:
445 if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
446 if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
447 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
448 else
449 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
450 return I40IW_SUCCESS;
451 }

Quoted 7 lines

The issue is that lines of code 447 and 449 are identical for different branches.
My question here is if one of the branches should be modified, or the entire _if_ statement replaced?
Maybe a patch like the following could be applied:

It looks like that you can replace I40IW_VCHNL_OP_GET_VER_V0 with I40IW_VCHNL_OP_GET_VER and get rid of all places with I40IW_VCHNL_OP_GET_VER_V0.

Thanks

Quoted 30 lines

index f4d1368..48fd327 100644

drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +1 -4

@@ -443,10 +443,7 @@ enum i40iw_status_code i40iw_vchnl_recv_pf(struct
 i40iw_sc_dev *dev,
        if (!dev->vchnl_up)
                return I40IW_ERR_NOT_READY;
        if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
-               if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
-               else
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
+               vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
                return I40IW_SUCCESS;
        }

        for (iw_vf_idx = 0; iw_vf_idx < I40IW_MAX_PE_ENABLED_VF_COUNT;

iw_vf_idx++) {

What do you think?

I'd really appreciate any comment on this.

Thank you!
--
Gustavo A. R. Silva

Chien Tin Tung

2017-05-18 15:03 UTC #2 lore raw

Quoted 18 lines · Leon Romanovsky

Quoted 17 lines · Gustavo A. R. Silva
Hello everybody,
While looking into Coverity ID 1362263 I ran into the following piece of code at drivers/infiniband/hw/i40iw/i40iw_virtchnl.c:445:
445 if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
446 if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
447 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
448 else
449 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
450 return I40IW_SUCCESS;
451 }
The issue is that lines of code 447 and 449 are identical for different branches.

Quoted 8 lines

Quoted 4 lines
My question here is if one of the branches should be modified, or the entire _if_ statement replaced?
Maybe a patch like the following could be applied:
It looks like that you can replace I40IW_VCHNL_OP_GET_VER_V0 with I40IW_VCHNL_OP_GET_VER and get rid of all places with I40IW_VCHNL_OP_GET_VER_V0.

No. I40IW_VCHNL_OP_GET_VER is iw_op_code and I40IW_VCHNL_OP_GET_VER_V0 is iw_op_ver two different things.

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Yuval Shaia

Reviewed-by

2017-05-18 06:00 UTC #3 lore raw

In reply to Gustavo A. R. Silva

Quoted 38 lines · Gustavo A. R. Silva

Hello everybody,
While looking into Coverity ID 1362263 I ran into the following piece of code at drivers/infiniband/hw/i40iw/i40iw_virtchnl.c:445:
445 if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
446 if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
447 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
448 else
449 vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
450 return I40IW_SUCCESS;
451 }
The issue is that lines of code 447 and 449 are identical for different branches.
My question here is if one of the branches should be modified, or the entire _if_ statement replaced?
Maybe a patch like the following could be applied:
index f4d1368..48fd327 100644
drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +1 -4
@@ -443,10 +443,7 @@ enum i40iw_status_code i40iw_vchnl_recv_pf(struct
 i40iw_sc_dev *dev,
        if (!dev->vchnl_up)
                return I40IW_ERR_NOT_READY;
        if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
-               if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
-               else
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
+               vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
                return I40IW_SUCCESS;
        }
        for (iw_vf_idx = 0; iw_vf_idx < I40IW_MAX_PE_ENABLED_VF_COUNT;

Quoted 3 lines

iw_vf_idx++) {
What do you think?

This looks like a nice catch!

Reviewed-by Yuval Shaia <[email protected]>

Quoted 10 lines

I'd really appreciate any comment on this.
Thank you!
--
Gustavo A. R. Silva

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Saleem, Shiraz

2017-05-18 14:35 UTC #4 lore raw

In reply to Gustavo A. R. Silva

DQo+IFN1YmplY3Q6IFtpbmZpbmliYW5kLWh3LWk0MGl3XSBxdWVzdGlvbiBhYm91dCBpZGVudGlj YWwgY29kZSBmb3IgZGlmZmVyZW50IGJyYW5jaGVzDQo+IA0KPiANCj4gaW5kZXggZjRkMTM2OC4u NDhmZDMyNyAxMDA2NDQNCj4gLS0tIGEvZHJpdmVycy9pbmZpbmliYW5kL2h3L2k0MGl3L2k0MGl3 X3ZpcnRjaG5sLmMNCj4gKysrIGIvZHJpdmVycy9pbmZpbmliYW5kL2h3L2k0MGl3L2k0MGl3X3Zp cnRjaG5sLmMNCj4gQEAgLTQ0MywxMCArNDQzLDcgQEAgZW51bSBpNDBpd19zdGF0dXNfY29kZSBp NDBpd192Y2hubF9yZWN2X3BmKHN0cnVjdA0KPiBpNDBpd19zY19kZXYgKmRldiwNCj4gICAgICAg ICAgaWYgKCFkZXYtPnZjaG5sX3VwKQ0KPiAgICAgICAgICAgICAgICAgIHJldHVybiBJNDBJV19F UlJfTk9UX1JFQURZOw0KPiAgICAgICAgICBpZiAodmNobmxfbXNnLT5pd19vcF9jb2RlID09IEk0 MElXX1ZDSE5MX09QX0dFVF9WRVIpIHsNCj4gLSAgICAgICAgICAgICAgIGlmICh2Y2hubF9tc2ct Pml3X29wX3ZlciAhPSBJNDBJV19WQ0hOTF9PUF9HRVRfVkVSX1YwKQ0KPiAtICAgICAgICAgICAg ICAgICAgICAgICB2Y2hubF9wZl9zZW5kX2dldF92ZXJfcmVzcChkZXYsIHZmX2lkLCB2Y2hubF9t c2cpOw0KPiAtICAgICAgICAgICAgICAgZWxzZQ0KPiAtICAgICAgICAgICAgICAgICAgICAgICB2 Y2hubF9wZl9zZW5kX2dldF92ZXJfcmVzcChkZXYsIHZmX2lkLCB2Y2hubF9tc2cpOw0KPiArICAg ICAgICAgICAgICAgdmNobmxfcGZfc2VuZF9nZXRfdmVyX3Jlc3AoZGV2LCB2Zl9pZCwgdmNobmxf bXNnKTsNCj4gICAgICAgICAgICAgICAgICByZXR1cm4gSTQwSVdfU1VDQ0VTUzsNCj4gICAgICAg ICAgfQ0KPiAgICAgICAgICBmb3IgKGl3X3ZmX2lkeCA9IDA7IGl3X3ZmX2lkeCA8IEk0MElXX01B WF9QRV9FTkFCTEVEX1ZGX0NPVU5UOw0KPiBpd192Zl9pZHgrKykgew0KPiANCj4gV2hhdCBkbyB5 b3UgdGhpbms/DQo+IA0KPiBJJ2QgcmVhbGx5IGFwcHJlY2lhdGUgYW55IGNvbW1lbnQgb24gdGhp cy4NCj4gDQpZZXMuIFRoaXMgZml4IGlzIGZpbmUuDQoNClNoaXJheg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Gustavo A. R. Silva

author

2017-05-18 17:54 UTC #5 lore raw

Quoting "Saleem, Shiraz" <[email protected]>:

Quoted 21 lines

Subject: [infiniband-hw-i40iw] question about identical code for different branches

index f4d1368..48fd327 100644

drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +1 -4

@@ -443,10 +443,7 @@ enum i40iw_status_code i40iw_vchnl_recv_pf(struct
 i40iw_sc_dev *dev,
         if (!dev->vchnl_up)
                 return I40IW_ERR_NOT_READY;
         if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
-               if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
-               else
-                       vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
+               vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
                 return I40IW_SUCCESS;
         }

         for (iw_vf_idx = 0; iw_vf_idx < I40IW_MAX_PE_ENABLED_VF_COUNT;

iw_vf_idx++) {

Quoted 6 lines

Quoted 4 lines
What do you think?
I'd really appreciate any comment on this.
Yes. This fix is fine.

I'll send a patch in a full and proper format shortly.

Thank you all for your comments.
--
Gustavo A. R. Silva

Gustavo A. R. Silva

author

2017-05-18 18:11 UTC #6 lore raw

PATCH 1/1 infiniband: hw: i40iw: fix duplicated code for different branches

Refactor code to avoid identical code for different branches.

Addresses-Coverity-ID: 1357356

Reviewed-by Yuval Shaia <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/infiniband/hw/i40iw/i40iw_virtchnl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +1 -4

index f4d1368..48fd327 100644
@@ -443,10 +443,7 @@ enum i40iw_status_code i40iw_vchnl_recv_pf(struct i40iw_sc_dev *dev,
 	if (!dev->vchnl_up)
 		return I40IW_ERR_NOT_READY;
 	if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
-		if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
-			vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
-		else
-			vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
+		vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
 		return I40IW_SUCCESS;
 	}
 	for (iw_vf_idx = 0; iw_vf_idx < I40IW_MAX_PE_ENABLED_VF_COUNT; iw_vf_idx++) {

Signature · 1 line

2.5.0

Shiraz Saleem

Acked-by on v1 1/1

2017-05-19 16:59 UTC #7 lore raw

Re: [PATCH] infiniband: hw: i40iw: fix duplicated code for different branches

Quoted 16 lines · Gustavo A. R. Silva

Refactor code to avoid identical code for different branches.
Addresses-Coverity-ID: 1357356
Reviewed-by Yuval Shaia <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/infiniband/hw/i40iw/i40iw_virtchnl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +0 -0
index f4d1368..48fd327 100644
@@ -443,10 +443,7 @@ enum i40iw_status_code i40iw_vchnl_recv_pf(struct i40iw_sc_dev *dev,
 	if (!dev->vchnl_up)
 		return I40IW_ERR_NOT_READY;

drivers/infiniband/hw/i40iw/i40iw_virtchnl.c enum i40iw_status_code i40iw_vchnl_recv_pf(struct i40iw_sc_dev *dev,

Quoted 10 lines

drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +1 -5

 	if (vchnl_msg->iw_op_code == I40IW_VCHNL_OP_GET_VER) {
-		if (vchnl_msg->iw_op_ver != I40IW_VCHNL_OP_GET_VER_V0)
-			vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
-		else
-			vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
+		vchnl_pf_send_get_ver_resp(dev, vf_id, vchnl_msg);
 		return I40IW_SUCCESS;
 	}
 	for (iw_vf_idx = 0; iw_vf_idx < I40IW_MAX_PE_ENABLED_VF_COUNT; iw_vf_idx++) {
--

Acked-by Shiraz Saleem <[email protected]>

Doug Ledford

Applied

2017-06-01 22:27 UTC #8 lore raw

Re: [PATCH] infiniband: hw: i40iw: fix duplicated code for different branches

In reply to Gustavo A. R. Silva

Quoted 5 lines · Gustavo A. R. Silva

Refactor code to avoid identical code for different branches.
Addresses-Coverity-ID: 1357356
Reviewed-by Yuval Shaia <[email protected]>
Signed-off-by Gustavo A. R. Silva <garsilva-L1vi/[email protected]>

Thanks, applied.

Signature · 9 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html