Re: [PATCH 4/4] scsi_debug: add resp_write_scat function

[Douglas Gilbert <dgilbert@interlog.com>]

v2 coming, addressing some of the points below. Also there is an
issue when fake_rw=1 .

Doug Gilbert

On 2017-12-12 03:47 PM, Douglas Gilbert wrote:
> On 2017-12-12 02:40 PM, Bart Van Assche wrote:
>> On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
>>> -static const struct opcode_info_t vl_iarr[1] = {    /* VARIABLE LENGTH */
>>> +static const struct opcode_info_t vl_iarr[2] = {    /* VARIABLE LENGTH */
>>
>> Please leave out the array size and let the compiler determine the array size.
> 
> I like the "belts and braces" approach. Especially if offsetting an
> array element by one position would silently destroy the parser
> (which is not the case with vl_iarr but is the case with a few
> other arrays).
> 
>>
>>>       {0, 0x7f, 0xb, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_dt0,
>>> -        NULL, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0xb, 0xfa,
>>> -           0, 0xff, 0xff, 0xff, 0xff} },    /* WRITE(32) */
>>> +        NULL, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0xb, 0xfa,
>>> +        0, 0xff, 0xff, 0xff, 0xff} },    /* WRITE(32) */
>>
>> Shouldn't this change have been included in the patch that fixes the group
>> number mask?
> 
> git add --patch
> 
> was used to break up a monolithic patch into 4 parts. However in
> the above case it refused to "split" the group_number part (shown)
> from the renaming of service actions (not shown above). At that
> point I swear at git and move on.
> 
> If folks think that hours are spent testing a kernel with 1/4 and
> 2/4 applied while 3/4 and 4/4 have not been applied then I think
> they are dreaming. IMO The split up is eye candy for reviewers.
> 
>>>   static const struct opcode_info_t maint_in_iarr[2] = {
>>> @@ -518,9 +523,10 @@ static const struct opcode_info_t 
>>> opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>>>           {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>>>       {1, 0x9e, 0x10, F_SA_LOW | F_D_IN, resp_readcap16, sa_in_iarr,
>>>           {16,  0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>>> -         0xff, 0xff, 0xff, 0x1, 0xc7} },    /* READ CAPACITY(16) */
>>> -    {0, 0, 0, F_INV_OP | FF_RESPOND, NULL, NULL, /* SA OUT */
>>> -        {0,  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>>> +         0xff, 0xff, 0xff, 0x1, 0xc7} },/* SA_IN(16), READ CAPACITY(16) */
>>
>> Shouldn't the above change be folded into one of the other patches?
> 
> I considered the patch adding resp_write_scat to the parsing table
> and the splitting SDEB_I_SERV_ACT_IN/OUT to its 12 and 16 byte cdb
> components as very closely related, so I put them together.
> 
>>> @@ -529,9 +535,9 @@ static const struct opcode_info_t 
>>> opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>>>       {0, 0x2f, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, NULL, NULL, /* VERIFY(10) */
>>>           {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7,
>>>            0, 0, 0, 0, 0, 0} },
>>> -    {1, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
>>> -        vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0x9, 0xfe, 0,
>>> -              0xff, 0xff, 0xff, 0xff} },/* VARIABLE LENGTH, READ(32) */
>>> +    {2, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
>>> +        vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0x9, 0xfe, 0,
>>> +        0xff, 0xff, 0xff, 0xff} },    /* VARIABLE LENGTH, READ(32) */
>>
>> Have you considered to use ARRAY_SIZE(vl_iarr) instead of hard-coding the array
>> size?
> 
> No. It could be done with the advantage of making the code
> a bit safer for someone who changes it, but it obfuscates
> the number elements in the bucket list (array) making it
> harder for the reader of the code (maybe).
> 
>>> +    if (cmd[0] == VARIABLE_LENGTH_CMD) {
>>> +        is_16 = false;
>>> +        wrprotect = (cmd[10] >> 5) & 0x7;
>>> +        lbdof = get_unaligned_be16(cmd + 12);
>>> +        num_lrd = get_unaligned_be16(cmd + 16);
>>> +        bt_len = get_unaligned_be32(cmd + 28);
>>> +        check_prot = false;
>>> +    } else {        /* that leaves WRITE SCATTERED(16) */
>>> +        is_16 = true;
>>> +        wrprotect = (cmd[2] >> 5) & 0x7;
>>> +        lbdof = get_unaligned_be16(cmd + 4);
>>> +        num_lrd = get_unaligned_be16(cmd + 8);
>>> +        bt_len = get_unaligned_be32(cmd + 10);
>>> +        check_prot = true;
>>> +    }
>>
>> It's not clear to me why check_prot is set to false for WRITE SCATTERED(32)
>> and set to true for WRITE SCATTERED(16)?
> 
> Me neither. I was just following what the code for WRITE(16 and 32)
> does. My guess is that the code in question is written by Martin P.
> who is effectively co-maintainer of this driver having written all
> the DIF and DIX stuff. Martin ... ?
> 
> Doug Gilbert
> 
>