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 micha...@cs.wisc.edu

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

Signed-off-by: Mike Christie micha...@cs.wisc.edu
---
   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-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 micha...@cs.wisc.edu

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

 Signed-off-by: Mike Christie micha...@cs.wisc.edu
 ---
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-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 micha...@cs.wisc.edu

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

 Signed-off-by: Mike Christie micha...@cs.wisc.edu
 ---
   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 micha...@cs.wisc.edu

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

Signed-off-by: Mike Christie micha...@cs.wisc.edu
---
  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

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

2014-07-12 Thread michaelc 
From: Mike Christie micha...@cs.wisc.edu

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

Signed-off-by: Mike Christie micha...@cs.wisc.edu
---
 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));
-- 
1.7.1

--
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