Skip to contentSkip to conversation

libosd: bug fixes

PATCH In review Boaz Harrosh 1 participant 5 messages Nov 8, 2009 – Nov 10, 2009 SCSI

lore
Share & tools
lore

Boaz Harrosh

author
#0 lore raw

cover libosd: bug fixes

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

author
#1 lore raw

PATCH 1/2 libosd: bug in osd_req_decode_sense_full()

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 <[email protected]>
Signed-off-by Boaz Harrosh <[email protected]>
---
 drivers/scsi/osd/osd_initiator.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
drivers/scsi/osd/osd_initiator.c +4 -3
index 60b7ca1..d504e82 100644
@@ -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",
Signature · 1 line
1.6.5.1
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/scsi/osd/osd_initiator.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/scsi/osd/osd_initiator.c +1 -0
index d504e82..26d8dc0 100644
@@ -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;
 		}
Signature · 1 line
1.6.5.1
author
#2 lore raw

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

In reply to Boaz Harrosh
Quoted 21 lines · 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 <[email protected]>
Signed-off-by Boaz Harrosh <[email protected]>
---
 drivers/scsi/osd/osd_initiator.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
drivers/scsi/osd/osd_initiator.c +0 -0
index 60b7ca1..d504e82 100644

drivers/scsi/osd/osd_initiator.c int osd_req_decode_sense_full(struct osd_request *or, · line 1586

Quoted 3 lines
drivers/scsi/osd/osd_initiator.c +0 -0
@@ -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

Quoted 22 lines
drivers/scsi/osd/osd_initiator.c +4 -3
-			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",
author
#3 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Boaz Harrosh <[email protected]>
---
 drivers/scsi/osd/osd_initiator.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
drivers/scsi/osd/osd_initiator.c +5 -4
index 60b7ca1..5e90d19 100644
@@ -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",
Signature · 1 line
1.6.5.1