Re: [PATCH 1/4] iscsi class: fix get_host_stats error handling

2014-08-01 Thread Mike Christie 
On 08/01/2014 08:31 AM, Hannes Reinecke wrote:
> On 08/01/2014 12:32 AM, Mike Christie wrote:
>> On 07/30/2014 07:50 AM, Hannes Reinecke wrote:
>>> On 07/12/2014 10:51 PM, micha...@cs.wisc.edu wrote:
 From: Mike Christie 

 iscsi_get_host_stats was dropping the error code returned
 by drivers like qla4xxx.

 Signed-off-by: Mike Christie 
 ---
drivers/scsi/scsi_transport_iscsi.c |4 
1 files changed, 4 insertions(+), 0 deletions(-)

 diff --git a/drivers/scsi/scsi_transport_iscsi.c
 b/drivers/scsi/scsi_transport_iscsi.c
 index b481e62..14bfa53 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));

>>> What happens with the skbhost_stats allocated earlier? Shouldn't it be
>>> freed here, too?
>>>
>>
>> You mean for this success path right. It is not freed here by the iscsi
>> code on purpose. For the code path here where we have successfully
>> called into the driver then a couple lines below we will do
>>
>> iscsi_multicast_skb() -> nlmsg_multicast()
>>
>> which will pass the skbhost_stats skb to the netlink layer. The
>> netlink/socket/skb code then frees it when it is done with it.
>>
> No, I meant the failure path.
> 
> You do an alloc_skb above, then get_host_stats failed, and you exit the
> function without freeing the skb.

Ah, actually I am freeing it, but I now realize with the wrong function,
so I think that is why I was confused by your comment.  Above I added a
kfree() of the skb. I should be using kfree_skb. Will fix it up. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] iscsi class: fix get_host_stats error handling

2014-08-01 Thread Hannes Reinecke 

On 08/01/2014 12:32 AM, Mike Christie wrote:

On 07/30/2014 07:50 AM, Hannes Reinecke wrote:

On 07/12/2014 10:51 PM, micha...@cs.wisc.edu wrote:

From: Mike Christie 

iscsi_get_host_stats was dropping the error code returned
by drivers like qla4xxx.

Signed-off-by: Mike Christie 
---
   drivers/scsi/scsi_transport_iscsi.c |4 
   1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c
b/drivers/scsi/scsi_transport_iscsi.c
index b481e62..14bfa53 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));


What happens with the skbhost_stats allocated earlier? Shouldn't it be
freed here, too?



You mean for this success path right. It is not freed here by the iscsi
code on purpose. For the code path here where we have successfully
called into the driver then a couple lines below we will do

iscsi_multicast_skb() -> nlmsg_multicast()

which will pass the skbhost_stats skb to the netlink layer. The
netlink/socket/skb code then frees it when it is done with it.


No, I meant the failure path.

You do an alloc_skb above, then get_host_stats failed, and you exit 
the function without freeing the skb.

Or do I miss something?


Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] iscsi class: fix get_host_stats error handling

2014-07-31 Thread Mike Christie 
On 07/30/2014 07:50 AM, Hannes Reinecke wrote:
> On 07/12/2014 10:51 PM, micha...@cs.wisc.edu wrote:
>> From: Mike Christie 
>>
>> iscsi_get_host_stats was dropping the error code returned
>> by drivers like qla4xxx.
>>
>> Signed-off-by: Mike Christie 
>> ---
>>   drivers/scsi/scsi_transport_iscsi.c |4 
>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c
>> b/drivers/scsi/scsi_transport_iscsi.c
>> index b481e62..14bfa53 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));
>>
> What happens with the skbhost_stats allocated earlier? Shouldn't it be
> freed here, too?
> 

You mean for this success path right. It is not freed here by the iscsi
code on purpose. For the code path here where we have successfully
called into the driver then a couple lines below we will do

iscsi_multicast_skb() -> nlmsg_multicast()

which will pass the skbhost_stats skb to the netlink layer. The
netlink/socket/skb code then frees it when it is done with it.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] iscsi class: fix get_host_stats error handling

2014-07-30 Thread Hannes Reinecke 

On 07/12/2014 10:51 PM, micha...@cs.wisc.edu wrote:

From: Mike Christie 

iscsi_get_host_stats was dropping the error code returned
by drivers like qla4xxx.

Signed-off-by: Mike Christie 
---
  drivers/scsi/scsi_transport_iscsi.c |4 
  1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index b481e62..14bfa53 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));

What happens with the skbhost_stats allocated earlier? Shouldn't it 
be freed here, too?


Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html