On 07/07/14 10:01 pm, "Mike Christie" <micha...@cs.wisc.edu> wrote:
>On 07/04/2014 03:37 PM, Julia Lawall wrote: >> >> >> On Fri, 4 Jul 2014, Elliott, Robert (Server Storage) wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi- >>>> ow...@vger.kernel.org] On Behalf Of Himangi Saraogi >>>> Sent: Friday, 04 July, 2014 1:28 PM >>>> To: Vikas Chaudhary; iscsi-dri...@qlogic.com; James E.J. Bottomley; >>>>linux- >>>> s...@vger.kernel.org; linux-ker...@vger.kernel.org >>>> Cc: julia.law...@lip6.fr >>>> Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure >>>> >>>> In this code, 0 is returned on memory allocation failure, even though >>>> other failures return -ENOMEM or other similar values. >>>> >>>> A simplified version of the Coccinelle semantic match that finds this >>>> problem is as follows: >>>> >>>> // <smpl> >>>> @@ >>>> expression ret; >>>> expression x,e1,e2,e3; >>>> identifier alloc; >>>> @@ >>>> >>>> ret = 0 >>>> ... when != ret = e1 >>>> *x = alloc(...) >>>> ... when != ret = e2 >>>> if (x == NULL) { ... when != ret = e3 >>>> return ret; >>>> } >>>> // </smpl> >>>> >>>> Signed-off-by: Himangi Saraogi <himangi...@gmail.com> >>>> Acked-by: Julia Lawall <julia.law...@lip6.fr> >>>> --- >>>> drivers/scsi/qla4xxx/ql4_os.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/scsi/qla4xxx/ql4_os.c >>>>b/drivers/scsi/qla4xxx/ql4_os.c >>>> index c5d9564..72ba671 100644 >>>> --- a/drivers/scsi/qla4xxx/ql4_os.c >>>> +++ b/drivers/scsi/qla4xxx/ql4_os.c >>>> @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct >>>>Scsi_Host >>>> *shost, char *buf, int len) >>>> if (!ql_iscsi_stats) { >>>> ql4_printk(KERN_ERR, ha, >>>> "Unable to allocate memory for iscsi stats\n"); >>>> + ret = -ENOMEM; >>>> goto exit_host_stats; >>>> } >>>> >>> >>> Also, the only caller of this function doesn't use the return >>> value - it's overwritten by another function call: >>> >>> drivers/scsi/scsi_transport_iscsi.c: >>> err = transport->get_host_stats(shost, buf, >>>host_stats_size); >>> >>> actual_size = nlmsg_total_size(sizeof(*ev) + >>>host_stats_size); >>> skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size)); >>> nlhhost_stats->nlmsg_len = actual_size; >>> >>> err = iscsi_multicast_skb(skbhost_stats, >>>ISCSI_NL_GRP_ISCSID, >>> GFP_KERNEL); >> >> Maybe that is intentional? > >I think it was a mistake. There is also one more issue in >qla4xxx_get_host_stats where it is returning an internal qlogic >specific error value. This patch should fix all issues. > > >[PATCH] qla4xxx/iscsi: fix get_host_stats error propagation > >This patch fixes 2 bugs. > >1. qla4xxx was not always returning -EXYZ error codes when >qla4xxx_get_host_stats failed. > >2. iscsi_get_host_stats was dropping the error code returned >by drivers like qla4xxx. > >Signed-off-by: Mike Christie <micha...@cs.wisc.edu> > >diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c >index 459b9f7..e4dc3e0 100644 >--- a/drivers/scsi/qla4xxx/ql4_os.c >+++ b/drivers/scsi/qla4xxx/ql4_os.c >@@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host >*shost, char *buf, int len) > if (!ql_iscsi_stats) { > ql4_printk(KERN_ERR, ha, > "Unable to allocate memory for iscsi stats\n"); >+ ret = -ENOMEM; > goto exit_host_stats; > } > >@@ -1058,6 +1059,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host >*shost, char *buf, int len) > if (ret != QLA_SUCCESS) { > ql4_printk(KERN_ERR, ha, > "Unable to retrieve iscsi stats\n"); >+ ret = -EIO; > goto exit_host_stats; > } > host_stats->mactx_frames = le64_to_cpu(ql_iscsi_stats->mac_tx_frames); >diff --git a/drivers/scsi/scsi_transport_iscsi.c >b/drivers/scsi/scsi_transport_iscsi.c >index 0102a2d..ea2996a 100644 >--- a/drivers/scsi/scsi_transport_iscsi.c >+++ b/drivers/scsi/scsi_transport_iscsi.c >@@ -3467,6 +3467,10 @@ iscsi_get_host_stats(struct iscsi_transport >*transport, struct nlmsghdr *nlh) > memset(buf, 0, host_stats_size); > > err = transport->get_host_stats(shost, buf, host_stats_size); >+ if (err) { >+ kfree(skbhost_stats); >+ goto exit_host_stats; >+ } > > actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size); > skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size)); Acked-by: Vikas Chaudhary <vikas.chaudh...@qlogic.com>
<<attachment: winmail.dat>>
