[PATCH 0/2] libosd: bug fixes

[PATCH 0/2] libosd: bug fixes

[Boaz Harrosh]

These are two additional bug-fix patches to libosd for the
2.6.33 merge window.

Please consider for submission on top of the previous 5

[PATCH 1/2] libosd: bug in osd_req_decode_sense_full()
[PATCH 2/2] libosd: Bugfix of error handling in attributes-list decoding

Thanks
Boaz

[PATCH 1/2] libosd: bug in osd_req_decode_sense_full()

[Boaz Harrosh]

The (never tested) osd_sense_attribute_identification case
has never worked. The loop was never advanced on.
Fix it to work as intended.

On 10/30/2009 04:39 PM, Roel Kluin wrote:
  I found this by code analysis, searching for while
  loops that test a local variable, but do not modify
  the variable.

Reported-by: Roel Kluin <roel.kluin@gmail.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 60b7ca1..d504e82 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1584,14 +1584,13 @@ int osd_req_decode_sense_full(struct osd_request *or,
 			struct osd_sense_attributes_data_descriptor
 				*osadd = cur_descriptor;
 			int len = min(cur_len, sense_len);
-			int i = 0;
 			struct osd_sense_attr *pattr = osadd->sense_attrs;
 
-			while (len < 0) {
+			while (len >= sizeof(*pattr)) {
 				u32 attr_page = be32_to_cpu(pattr->attr_page);
 				u32 attr_id = be32_to_cpu(pattr->attr_id);
 
-				if (i++ == 0) {
+				if (!osi->attr.attr_page) {
 					osi->attr.attr_page = attr_page;
 					osi->attr.attr_id = attr_id;
 				}
@@ -1602,6 +1601,8 @@ int osd_req_decode_sense_full(struct osd_request *or,
 					bad_attr_list++;
 					max_attr--;
 				}
+
+				len -= sizeof(*pattr);
 				OSD_SENSE_PRINT2(
 					"osd_sense_attribute_identification"
 					"attr_page=0x%x attr_id=0x%x\n",
-- 
1.6.5.1

[PATCH 2/2] libosd: Bugfix of error handling in attributes-list decoding

[Boaz Harrosh]

When an error was detected in an attribute list, do to
a target bug. We would print an error but spin endlessly
regardless. Fix it.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index d504e82..26d8dc0 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1167,6 +1167,7 @@ int osd_req_decode_get_attr_list(struct osd_request *or,
 				"c=%d r=%d n=%d\n",
 				cur_bytes, returned_bytes, n);
 			oa->val_ptr = NULL;
+			cur_bytes = returned_bytes; /* break the caller loop */
 			break;
 		}
 
-- 
1.6.5.1

Re: [osd-dev] [PATCH 1/2] libosd: bug in osd_req_decode_sense_full()

[Boaz Harrosh]

On 11/08/2009 02:29 PM, Boaz Harrosh wrote:
> 
> The (never tested) osd_sense_attribute_identification case
> has never worked. The loop was never advanced on.
> Fix it to work as intended.
> 
> On 10/30/2009 04:39 PM, Roel Kluin wrote:
>   I found this by code analysis, searching for while
>   loops that test a local variable, but do not modify
>   the variable.
> 
> Reported-by: Roel Kluin <roel.kluin@gmail.com>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/osd/osd_initiator.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index 60b7ca1..d504e82 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1584,14 +1584,13 @@ int osd_req_decode_sense_full(struct osd_request *or,
>  			struct osd_sense_attributes_data_descriptor
>  				*osadd = cur_descriptor;
>  			int len = min(cur_len, sense_len);

len should be unsigned for the compare with sizeof() below.

I'll repost a second version as reply.

Boaz
> -			int i = 0;
>  			struct osd_sense_attr *pattr = osadd->sense_attrs;
>  
> -			while (len < 0) {
> +			while (len >= sizeof(*pattr)) {
>  				u32 attr_page = be32_to_cpu(pattr->attr_page);
>  				u32 attr_id = be32_to_cpu(pattr->attr_id);
>  
> -				if (i++ == 0) {
> +				if (!osi->attr.attr_page) {
>  					osi->attr.attr_page = attr_page;
>  					osi->attr.attr_id = attr_id;
>  				}
> @@ -1602,6 +1601,8 @@ int osd_req_decode_sense_full(struct osd_request *or,
>  					bad_attr_list++;
>  					max_attr--;
>  				}
> +
> +				len -= sizeof(*pattr);
>  				OSD_SENSE_PRINT2(
>  					"osd_sense_attribute_identification"
>  					"attr_page=0x%x attr_id=0x%x\n",

[PATCH1/2 version2] libosd: bug in osd_req_decode_sense_full()

[Boaz Harrosh]

The (never tested) osd_sense_attribute_identification case
has never worked. The loop was never advanced on.
Fix it to work as intended.

On 10/30/2009 04:39 PM, Roel Kluin wrote:
  I found this by code analysis, searching for while
  loops that test a local variable, but do not modify
  the variable.

Reported-by: Roel Kluin <roel.kluin@gmail.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 60b7ca1..5e90d19 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1583,15 +1583,14 @@ int osd_req_decode_sense_full(struct osd_request *or,
 		{
 			struct osd_sense_attributes_data_descriptor
 				*osadd = cur_descriptor;
-			int len = min(cur_len, sense_len);
-			int i = 0;
+			unsigned len = min(cur_len, sense_len);
 			struct osd_sense_attr *pattr = osadd->sense_attrs;
 
-			while (len < 0) {
+			while (len >= sizeof(*pattr)) {
 				u32 attr_page = be32_to_cpu(pattr->attr_page);
 				u32 attr_id = be32_to_cpu(pattr->attr_id);
 
-				if (i++ == 0) {
+				if (!osi->attr.attr_page) {
 					osi->attr.attr_page = attr_page;
 					osi->attr.attr_id = attr_id;
 				}
@@ -1602,6 +1601,8 @@ int osd_req_decode_sense_full(struct osd_request *or,
 					bad_attr_list++;
 					max_attr--;
 				}
+
+				len -= sizeof(*pattr);
 				OSD_SENSE_PRINT2(
 					"osd_sense_attribute_identification"
 					"attr_page=0x%x attr_id=0x%x\n",
-- 
1.6.5.1