On Sun, 2018-03-18 at 21:59 +0100, Douglas Gilbert wrote:
> Break out several interwined paths when cmd->result is non zero and
^^^^^^^^^^
intertwined?
> place them in scsi_io_completion_nz_result helper function. The logic
> is not changed.
> [ ... ]
> 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 sounds like bad advice to me ... I think the constructs in the current
version of this patch may lead to future maintainability problems. E.g. the
special value BLK_STS_NOTSUPP occurs in two different places. If anyone ever
wants to use another special value BLK_STS_NOTSUPP will have to be changed
into something else at two different locations. Additionally, if
__scsi_error_from_host_byte() ever would be modified to return BLK_STS_NOTSUPP
then the code introduced by this patch will break in a subtle way and it will
be really hard to figure out what went wrong and why something went wrong.
The approach of this patch however looks fine to me. scsi_io_completion() is
too long so I think it's certainly a good idea to split it into multiple
functions.
Thanks,
Bart.
