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@xxxxxxxxxxxx> 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 > > - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
