On Tue, 2018-03-27 at 15:05 -0400, Douglas Gilbert wrote:
> A reviewer requested the original helper function's two return values
> be reduced to one: the blk_stat variable. This required a hack to
> differentiate the default setting of blk_stat (BLK_STS_OK) from the case
> when the helper assigns BLK_STS_OK as the return value. The hack is to
> return the otherwise unused BLK_STS_NOTSUPP value as an indication that
> the helper didn't change anything. That hack was judged by another
> reviewer to be worse that the "two return values" ugliness it was
> trying to address. So back to the original "two return values" solution.
> Returning a structure containing result and blk_stat is another
> possibility but returning structures is frowned upon in some circles.
It seems to me like this version of this patch is such that the above comment
no longer applies?
> + sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
> + about_current = sense_valid ? !scsi_sense_is_deferred(&sshdr) : true;
> +
> + if (blk_rq_is_passthrough(req)) {
> + if (sense_valid) {
> + /*
> + * SG_IO wants current and deferred errors
> + */
> + scsi_req(req)->sense_len =
> + min(8 + cmd->sense_buffer[7],
> + SCSI_SENSE_BUFFERSIZE);
> + }
> + if (about_current)
> + *blk_statp = __scsi_error_from_host_byte(cmd, result);
This patch does more than just moving code. E.g. the 'about_current'
variable is new and does not match to any variable in the existing code.
Please separate code movement and code refactoring.
Thanks,
Bart.
