| Message ID | 20200825155142.349651-1-alex.dewar90@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IB/qib: remove superfluous fallthrough statements | expand |
On 8/25/20 10:51, Alex Dewar wrote: > Commit 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") > erroneously marked a couple of switch cases as /* FALLTHROUGH */, which > were later converted to fallthrough statements by commit df561f6688fe > ("treewide: Use fallthrough pseudo-keyword"). This triggered a Coverity > warning about unreachable code. > > Remove the fallthrough statements and replace the mass of gotos with > simple return statements to make the code terser and less bug-prone. > This should be split up into two separate patches: one to address the fallthrough markings, and another one for the gotos. Thanks -- Gustavo
On Tue, 2020-08-25 at 11:19 -0500, Gustavo A. R. Silva wrote: > > On 8/25/20 10:51, Alex Dewar wrote: > > Commit 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") > > erroneously marked a couple of switch cases as /* FALLTHROUGH */, which > > were later converted to fallthrough statements by commit df561f6688fe > > ("treewide: Use fallthrough pseudo-keyword"). This triggered a Coverity > > warning about unreachable code. > > > > Remove the fallthrough statements and replace the mass of gotos with > > simple return statements to make the code terser and less bug-prone. > > > > This should be split up into two separate patches: one to address the > fallthrough markings, and another one for the gotos. I don't think it's necessary to break this into multiple patches. Logical changes in a single patch are just fine, micro patches aren't that useful.
On Tue, 2020-08-25 at 11:49 -0500, Gustavo A. R. Silva wrote: > > On 8/25/20 11:26, Joe Perches wrote: > > On Tue, 2020-08-25 at 11:19 -0500, Gustavo A. R. Silva wrote: > > > On 8/25/20 10:51, Alex Dewar wrote: > > > > Commit 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") > > > > erroneously marked a couple of switch cases as /* FALLTHROUGH */, which > > > > were later converted to fallthrough statements by commit df561f6688fe > > > > ("treewide: Use fallthrough pseudo-keyword"). This triggered a Coverity > > > > warning about unreachable code. > > > > > > > > Remove the fallthrough statements and replace the mass of gotos with > > > > simple return statements to make the code terser and less bug-prone. > > > > > > > > > > This should be split up into two separate patches: one to address the > > > fallthrough markings, and another one for the gotos. > > > > I don't think it's necessary to break this into multiple patches. > > Logical changes in a single patch are just fine, micro patches > > aren't that useful. > > > > There is a reason for this. Read the changelog text and review the patch. What makes you think I didn't already do that? I think your desire for micropatches is unnecessary.
On 8/25/20 11:26, Joe Perches wrote: > On Tue, 2020-08-25 at 11:19 -0500, Gustavo A. R. Silva wrote: >> >> On 8/25/20 10:51, Alex Dewar wrote: >>> Commit 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") >>> erroneously marked a couple of switch cases as /* FALLTHROUGH */, which >>> were later converted to fallthrough statements by commit df561f6688fe >>> ("treewide: Use fallthrough pseudo-keyword"). This triggered a Coverity >>> warning about unreachable code. >>> >>> Remove the fallthrough statements and replace the mass of gotos with >>> simple return statements to make the code terser and less bug-prone. >>> >> >> This should be split up into two separate patches: one to address the >> fallthrough markings, and another one for the gotos. > > I don't think it's necessary to break this into multiple patches. > Logical changes in a single patch are just fine, micro patches > aren't that useful. > There is a reason for this. Read the changelog text and review the patch. Thanks -- Gustavo
On 25/08/2020 17:49, Gustavo A. R. Silva wrote: > > On 8/25/20 11:26, Joe Perches wrote: >> On Tue, 2020-08-25 at 11:19 -0500, Gustavo A. R. Silva wrote: >>> On 8/25/20 10:51, Alex Dewar wrote: >>>> Commit 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") >>>> erroneously marked a couple of switch cases as /* FALLTHROUGH */, which >>>> were later converted to fallthrough statements by commit df561f6688fe >>>> ("treewide: Use fallthrough pseudo-keyword"). This triggered a Coverity >>>> warning about unreachable code. >>>> >>>> Remove the fallthrough statements and replace the mass of gotos with >>>> simple return statements to make the code terser and less bug-prone. >>>> >>> This should be split up into two separate patches: one to address the >>> fallthrough markings, and another one for the gotos. >> I don't think it's necessary to break this into multiple patches. >> Logical changes in a single patch are just fine, micro patches >> aren't that useful. >> > There is a reason for this. Read the changelog text and review the patch. > > Thanks > -- > Gustavo I think it probably does make sense as two patches. I'll do a resend.
On 8/25/20 11:47, Joe Perches wrote: > On Tue, 2020-08-25 at 11:49 -0500, Gustavo A. R. Silva wrote: >> >> On 8/25/20 11:26, Joe Perches wrote: >>> On Tue, 2020-08-25 at 11:19 -0500, Gustavo A. R. Silva wrote: >>>> On 8/25/20 10:51, Alex Dewar wrote: >>>>> Commit 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") >>>>> erroneously marked a couple of switch cases as /* FALLTHROUGH */, which >>>>> were later converted to fallthrough statements by commit df561f6688fe >>>>> ("treewide: Use fallthrough pseudo-keyword"). This triggered a Coverity >>>>> warning about unreachable code. >>>>> >>>>> Remove the fallthrough statements and replace the mass of gotos with >>>>> simple return statements to make the code terser and less bug-prone. >>>>> >>>> >>>> This should be split up into two separate patches: one to address the >>>> fallthrough markings, and another one for the gotos. >>> >>> I don't think it's necessary to break this into multiple patches. >>> Logical changes in a single patch are just fine, micro patches >>> aren't that useful. >>> >> >> There is a reason for this. Read the changelog text and review the patch. > > What makes you think I didn't already do that? > You would have noticed this should be two patches. > I think your desire for micropatches is unnecessary. > You might be generalizing. My 'desire' here is justified and specific. Thanks -- Gustavo
On Tue, 2020-08-25 at 12:01 -0500, Gustavo A. R. Silva wrote: > On 8/25/20 11:47, Joe Perches wrote [] > You would have noticed this should be two patches. That's interpretational. > > I think your desire for micropatches is unnecessary. > > > You might be generalizing. My 'desire' here is justified and specific. And to date undescribed.
diff --git a/drivers/infiniband/hw/qib/qib_mad.c b/drivers/infiniband/hw/qib/qib_mad.c index e7789e724f56..f83e331977f8 100644 --- a/drivers/infiniband/hw/qib/qib_mad.c +++ b/drivers/infiniband/hw/qib/qib_mad.c @@ -2293,76 +2293,50 @@ static int process_cc(struct ib_device *ibdev, int mad_flags, struct ib_mad *out_mad) { struct ib_cc_mad *ccp = (struct ib_cc_mad *)out_mad; - int ret; - *out_mad = *in_mad; if (ccp->class_version != 2) { ccp->status |= IB_SMP_UNSUP_VERSION; - ret = reply((struct ib_smp *)ccp); - goto bail; + return reply((struct ib_smp *)ccp); } switch (ccp->method) { case IB_MGMT_METHOD_GET: switch (ccp->attr_id) { case IB_CC_ATTR_CLASSPORTINFO: - ret = cc_get_classportinfo(ccp, ibdev); - goto bail; - + return cc_get_classportinfo(ccp, ibdev); case IB_CC_ATTR_CONGESTION_INFO: - ret = cc_get_congestion_info(ccp, ibdev, port); - goto bail; - + return cc_get_congestion_info(ccp, ibdev, port); case IB_CC_ATTR_CA_CONGESTION_SETTING: - ret = cc_get_congestion_setting(ccp, ibdev, port); - goto bail; - + return cc_get_congestion_setting(ccp, ibdev, port); case IB_CC_ATTR_CONGESTION_CONTROL_TABLE: - ret = cc_get_congestion_control_table(ccp, ibdev, port); - goto bail; - - fallthrough; + return cc_get_congestion_control_table(ccp, ibdev, port); default: ccp->status |= IB_SMP_UNSUP_METH_ATTR; - ret = reply((struct ib_smp *) ccp); - goto bail; + return reply((struct ib_smp *) ccp); } - case IB_MGMT_METHOD_SET: switch (ccp->attr_id) { case IB_CC_ATTR_CA_CONGESTION_SETTING: - ret = cc_set_congestion_setting(ccp, ibdev, port); - goto bail; - + return cc_set_congestion_setting(ccp, ibdev, port); case IB_CC_ATTR_CONGESTION_CONTROL_TABLE: - ret = cc_set_congestion_control_table(ccp, ibdev, port); - goto bail; - - fallthrough; + return cc_set_congestion_control_table(ccp, ibdev, port); default: ccp->status |= IB_SMP_UNSUP_METH_ATTR; - ret = reply((struct ib_smp *) ccp); - goto bail; + return reply((struct ib_smp *) ccp); } - case IB_MGMT_METHOD_GET_RESP: /* * The ib_mad module will call us to process responses * before checking for other consumers. * Just tell the caller to process it normally. */ - ret = IB_MAD_RESULT_SUCCESS; - goto bail; - - case IB_MGMT_METHOD_TRAP: - default: - ccp->status |= IB_SMP_UNSUP_METHOD; - ret = reply((struct ib_smp *) ccp); + return IB_MAD_RESULT_SUCCESS; } -bail: - return ret; + /* method is unsupported */ + ccp->status |= IB_SMP_UNSUP_METHOD; + return reply((struct ib_smp *) ccp); } /**
Commit 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") erroneously marked a couple of switch cases as /* FALLTHROUGH */, which were later converted to fallthrough statements by commit df561f6688fe ("treewide: Use fallthrough pseudo-keyword"). This triggered a Coverity warning about unreachable code. Remove the fallthrough statements and replace the mass of gotos with simple return statements to make the code terser and less bug-prone. Addresses-Coverity: ("Unreachable code") Fixes: 36a8f01cd24b ("IB/qib: Add congestion control agent implementation") Signed-off-by: Alex Dewar <alex.dewar90@gmail.com> --- drivers/infiniband/hw/qib/qib_mad.c | 52 ++++++++--------------------- 1 file changed, 13 insertions(+), 39 deletions(-)