[PATCH] scsi: Fix printing of variable length commands

[PATCH] scsi: Fix printing of variable length commands

[Martin K. Petersen]

The MAINTENANCE IN array is incorrectly used when decoding variable
length commands.  Use the right array.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 9129bcf..db68e3b 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -219,7 +219,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len)
 			break;
 		}
 		sa = (cdbp[8] << 8) + cdbp[9];
-		name = get_sa_name(maint_in_arr, MAINT_IN_SZ, sa);
+		name = get_sa_name(variable_length_arr, VARIABLE_LENGTH_SZ, sa);
 		if (name) {
 			printk("%s", name);
 			if ((cdb_len > 0) && (len != cdb_len))

Re: [PATCH] scsi: Fix printing of variable length commands

[Boaz Harrosh]

On 01/19/2010 01:03 AM, Martin K. Petersen wrote:
> 
> The MAINTENANCE IN array is incorrectly used when decoding variable
> length commands.  Use the right array.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> ---
> 
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 9129bcf..db68e3b 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -219,7 +219,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len)
>  			break;
>  		}
>  		sa = (cdbp[8] << 8) + cdbp[9];
> -		name = get_sa_name(maint_in_arr, MAINT_IN_SZ, sa);
> +		name = get_sa_name(variable_length_arr, VARIABLE_LENGTH_SZ, sa);
>  		if (name) {
>  			printk("%s", name);
>  			if ((cdb_len > 0) && (len != cdb_len))
> --

If you are already at it could you also add the things that bothered me? I did stare
at this function before, but did not see the bug you are fixing, thanks.
(Did see other less important stuff)

(Below also includes your change)
---
diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 9129bcf..6c3c31e 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -218,19 +218,16 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len)
 			       "len=%d ext_len=%d", len, cdb_len);
 			break;
 		}
-		sa = (cdbp[8] << 8) + cdbp[9];
-		name = get_sa_name(maint_in_arr, MAINT_IN_SZ, sa);
+		sa = be16_to_cpu(
+			((struct scsi_varlen_cdb_hdr *)cdbp)->service_action);
+		name = get_sa_name(variable_length_arr, VARIABLE_LENGTH_SZ, sa);
 		if (name) {
 			printk("%s", name);
-			if ((cdb_len > 0) && (len != cdb_len))
-				printk(", in_cdb_len=%d, ext_len=%d",
-				       len, cdb_len);
 		} else {
 			printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
-			if ((cdb_len > 0) && (len != cdb_len))
-				printk(", in_cdb_len=%d, ext_len=%d",
-				       len, cdb_len);
 		}
+		if ((cdb_len > 0) && (len != cdb_len))
+			printk(", in_cdb_len=%d, ext_len=%d", len, cdb_len);
 		break;
 	case MAINTENANCE_IN:
 		sa = cdbp[1] & 0x1f;

Re: [PATCH] scsi: Fix printing of variable length commands

[Martin K. Petersen]

>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:

Boaz> If you are already at it could you also add the things that
Boaz> bothered me? I did stare at this function before, but did not see
Boaz> the bug you are fixing, thanks.  (Did see other less important
Boaz> stuff)

I'm ok with cleaning up the redundant statements even though it's a bit
of churn in what's otherwise a pure bug fix patch.  Simply on the
grounds that it's silly code.


> - sa = (cdbp[8] << 8) + cdbp[9];
> + sa = be16_to_cpu(((struct scsi_varlen_cdb_hdr *)cdbp)->service_action);

Here, however, I find the original code far superior.  I know what
you're doing and why but I find the old code a bazillion times easier to
read.  Short and to the point.  No complex typecasts and I don't have to
go hunting for varlen_cdb_hdr struct definitions.  I dive straight into
SBC and see that bytes 8 and 9 in the CDB constitute the service action.
Done!

I know that the header looks the same for all variable length commands.
But we don't have a struct scsi_cdb_header with an "operation code"
entry pointing to cmnd[0] either.  Nor do we have headers for the
MAINTENANCE and SERVICE ACTION commands.  To me a CDB is a byte stream
and not a bunch of structs.

Anyway.  I know where you stand on this :)


scsi: Fix printing of variable length commands

We dereferenced the MAINTENANCE IN array when decoding variable length
commands.  Use the right array.  Also consolidate identical if
statements below.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 9129bcf..7092ff6 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -219,18 +219,15 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len)
 			break;
 		}
 		sa = (cdbp[8] << 8) + cdbp[9];
-		name = get_sa_name(maint_in_arr, MAINT_IN_SZ, sa);
-		if (name) {
+		name = get_sa_name(variable_length_arr, VARIABLE_LENGTH_SZ, sa);
+		if (name)
 			printk("%s", name);
-			if ((cdb_len > 0) && (len != cdb_len))
-				printk(", in_cdb_len=%d, ext_len=%d",
-				       len, cdb_len);
-		} else {
+		else
 			printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
-			if ((cdb_len > 0) && (len != cdb_len))
-				printk(", in_cdb_len=%d, ext_len=%d",
-				       len, cdb_len);
-		}
+
+		if ((cdb_len > 0) && (len != cdb_len))
+			printk(", in_cdb_len=%d, ext_len=%d", len, cdb_len);
+
 		break;
 	case MAINTENANCE_IN:
 		sa = cdbp[1] & 0x1f;

Re: [PATCH] scsi: Fix printing of variable length commands

[Boaz Harrosh]

On 01/20/2010 09:17 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
> Boaz> If you are already at it could you also add the things that
> Boaz> bothered me? I did stare at this function before, but did not see
> Boaz> the bug you are fixing, thanks.  (Did see other less important
> Boaz> stuff)
> 
> I'm ok with cleaning up the redundant statements even though it's a bit
> of churn in what's otherwise a pure bug fix patch.  Simply on the
> grounds that it's silly code.
> 
> 
>> - sa = (cdbp[8] << 8) + cdbp[9];
>> + sa = be16_to_cpu(((struct scsi_varlen_cdb_hdr *)cdbp)->service_action);
> 
> Here, however, I find the original code far superior.  I know what
> you're doing and why but I find the old code a bazillion times easier to
> read.  Short and to the point.  No complex typecasts and I don't have to
> go hunting for varlen_cdb_hdr struct definitions.  I dive straight into
> SBC and see that bytes 8 and 9 in the CDB constitute the service action.
> Done!
> 

At least I can go hunting for varlen_cdb_hdr, which is a simple right-click,
ctags-lookup, and actually see the exact definition of the complete header.

With SBC I need to fire up acroread, search for the right section in a 500 pages
document and then start all these speculations of how a BE int in scsi is, first
byte need shifting, or the other way around.
And that is after I was able to register, pays large sums of money before I was
even able to read it. (And at the end produce code that in some systems is 90 times
slower, yes 90 times slower, ok for a be64)

Come on I give you the SBC on a silver plater, not in English, in C. And in the
same editor, and in git, already downloaded togther with your Kernel. You can't
really be serious about that. Except, that bad habits die hard.

> I know that the header looks the same for all variable length commands.
> But we don't have a struct scsi_cdb_header with an "operation code"
> entry pointing to cmnd[0] either.  Nor do we have headers for the
> MAINTENANCE and SERVICE ACTION commands.  To me a CDB is a byte stream
> and not a bunch of structs.
> 

Sure you do, why? just look harder it's there.
And, No it is not a stream by any definition of any language. Please?

Two lines up we are using scsi_varlen_cdb_length() which uses the same technic
so I thought it might be appropriate.

> Anyway.  I know where you stand on this :)
> 

That said, I know it's just a lost cause. At first I thought I should just ignore
it. But it's early in the morning, and before my first coffee, so I need to pick a
fight with someone... Thanks body ;)

> 
> scsi: Fix printing of variable length commands
> 
> We dereferenced the MAINTENANCE IN array when decoding variable length
> commands.  Use the right array.  Also consolidate identical if
> statements below.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 9129bcf..7092ff6 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -219,18 +219,15 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len)
>  			break;
>  		}
>  		sa = (cdbp[8] << 8) + cdbp[9];
> -		name = get_sa_name(maint_in_arr, MAINT_IN_SZ, sa);
> -		if (name) {
> +		name = get_sa_name(variable_length_arr, VARIABLE_LENGTH_SZ, sa);
> +		if (name)
>  			printk("%s", name);
> -			if ((cdb_len > 0) && (len != cdb_len))
> -				printk(", in_cdb_len=%d, ext_len=%d",
> -				       len, cdb_len);
> -		} else {
> +		else
>  			printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
> -			if ((cdb_len > 0) && (len != cdb_len))
> -				printk(", in_cdb_len=%d, ext_len=%d",
> -				       len, cdb_len);
> -		}
> +
> +		if ((cdb_len > 0) && (len != cdb_len))
> +			printk(", in_cdb_len=%d, ext_len=%d", len, cdb_len);
> +
>  		break;
>  	case MAINTENANCE_IN:
>  		sa = cdbp[1] & 0x1f;
>