| Message ID | 20170517170654.Horde.cfktFjC4G4wPJvJ8X1ZyUvW@gator4166.hostgator.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, May 17, 2017 at 05:06:54PM -0500, Gustavo A. R. Silva wrote: > > 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: 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 > > index f4d1368..48fd327 100644 > --- a/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c > +++ b/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c > @@ -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 > > > >
On Wed, May 17, 2017 at 05:06:54PM -0500, Gustavo A. R. Silva wrote: > > 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 > --- a/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c > +++ b/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c > @@ -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? This looks like a nice catch! Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> > > I'd really appreciate any comment on this. > > Thank you! > -- > Gustavo A. R. Silva > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thu, May 18, 2017 at 08:00:29AM +0300, Leon Romanovsky wrote: > On Wed, May 17, 2017 at 05:06:54PM -0500, Gustavo A. R. Silva wrote: > > > > 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: > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting "Saleem, Shiraz" <shiraz.saleem@intel.com>: >> Subject: [infiniband-hw-i40iw] question about identical code for >> different branches >> >> >> index f4d1368..48fd327 100644 >> --- a/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c >> +++ b/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c >> @@ -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. >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c +++ b/drivers/infiniband/hw/i40iw/i40iw_virtchnl.c @@ -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; }
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html