| Message ID | 6973844a1532ec2dc8e86f3533362e79d78ed774.1618132821.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: qla2xxx: Re-use existing error handling path | expand |
On 4/11/21 2:21 AM, Christophe JAILLET wrote: > The code above this hunk looks spurious to me. > > It looks like an error handling path (i.e. > "if (response_len > bsg_job->reply_payload.payload_len)") > but returns 0, which is the initial value of 'ret'. > > Shouldn't we have ret = -<something> here? Hmm ... if I read that code path correctly it is on purpose that that code path returns 0 and error reporting happens by reporting the EXT_STATUS_BUFFER_TOO_SMALL error code to user space. > --- > drivers/scsi/qla2xxx/qla_bsg.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c > index aef2f7cc89d3..d42b2ad84049 100644 > --- a/drivers/scsi/qla2xxx/qla_bsg.c > +++ b/drivers/scsi/qla2xxx/qla_bsg.c > @@ -2585,8 +2585,8 @@ qla2x00_get_host_stats(struct bsg_job *bsg_job) > > data = kzalloc(response_len, GFP_KERNEL); > if (!data) { > - kfree(req_data); > - return -ENOMEM; > + ret = -ENOMEM; > + goto host_stat_out; > } > > ret = qla2xxx_get_ini_stats(fc_bsg_to_shost(bsg_job), req_data->stat_type, Since the above looks good to me: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Sun, 11 Apr 2021 11:21:40 +0200, Christophe JAILLET wrote: > There is no need to duplicate some code, use the existing error handling > path to free some resources. > This is more future-proof. Applied to 5.13/scsi-queue, thanks! [1/1] scsi: qla2xxx: Re-use existing error handling path https://git.kernel.org/mkp/scsi/c/5dc3468888f8
diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c index aef2f7cc89d3..d42b2ad84049 100644 --- a/drivers/scsi/qla2xxx/qla_bsg.c +++ b/drivers/scsi/qla2xxx/qla_bsg.c @@ -2585,8 +2585,8 @@ qla2x00_get_host_stats(struct bsg_job *bsg_job) data = kzalloc(response_len, GFP_KERNEL); if (!data) { - kfree(req_data); - return -ENOMEM; + ret = -ENOMEM; + goto host_stat_out; } ret = qla2xxx_get_ini_stats(fc_bsg_to_shost(bsg_job), req_data->stat_type,
There is no need to duplicate some code, use the existing error handling path to free some resources. This is more future-proof. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- The code above this hunk looks spurious to me. It looks like an error handling path (i.e. "if (response_len > bsg_job->reply_payload.payload_len)") but returns 0, which is the initial value of 'ret'. Shouldn't we have ret = -<something> here? --- drivers/scsi/qla2xxx/qla_bsg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)