Re: [PATCH ver4 1/5] scsi_error: code cleanup before refactoring of scsi_send_eh_cmnd()

[Boaz Harrosh <bharrosh@panasas.com>]

I will send a ver4 of these patches as reply to base patches
Meanwhile I have some comments below.

Thanks for the review.

On Wed, Oct 03 2007 at 17:55 +0200, James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> You've altered the signature and function of the routine here, this
> needs to be documented by updating the docbook description above the
> function.  (I know you do it later, but each changeset should really be
> logically complete).
> 
Done, thanks

>> +		memset(scmd->cmnd, 0, 6);
> 
> This will lead to subtle bugs: some devices (notably ATAPI) have a fixed
> command slot they will copy this into.  You have potentially trailing
> bytes of junk that this could pick up ... we have ATAPI devices known to
> fall over if they see trailing junk in the command.  Just consolidate
> the scmd->cmnd memsets to
> 
> memset(scmd->cmnd, 0, sizeof(scmd->cmnd);
> 
> outside of the ifs
> 
I wanted to have a fixture where if @cmnd and @sense_bytes are Zero than
scsi_cmnd->cmnd is not touched at all. So I fixed it by doing if (@cmnd)
only in the second arm of the if and fixed the above to what you said.
(See patch that follows)

> 
> Moving this to the sense_bytes specific piece of the if also introduces
> a subtle bug:  If the driver doesn't do auto request sense and the
> command completes with CHECK CONDITION, the sense checking routine will
> potentially pick up stale sense data here
> 
Good call thanks, done.

> 
> Other than the three comments, this looks OK.
> 
> James
> 
>