[stgt] [PATCH] Fix ModeSense. ModeSense never returns residuals. Additionally fix the two arguments that were swapped in set_byte_safe.

[ronnie sahlberg]

Alexander,

You are right, I misread/misremembered rfc5048/3.1
I have re-sent a patch that only fixes the swapped parameters.



I added a test for residuals and they work correctly as is :

$ ./bin/iscsi-test-cu iscsi://127.0.0.1/iqn.ronnie.test/1 --test
SCSI.ModeSense6.Residuals -V

     CUnit - A Unit testing framework for C - Version 2.1-0
     http://cunit.sourceforge.net/


Suite: ModeSense6
  Test: Residuals ...
    Test of MODESENSE6 Residuals
    MODESENSE6 command should not result in any residuals
    Try a MODESENSE6 command with 4 bytes of transfer length and
verify that we dont get residuals.
    [SUCCESS] All Pages fetched.
    Verify that we got at most 4 bytes of DATA-IN
    [SUCCESS] <= 4 bytes of DATA-IN received.
    Verify residual overflow flag not set
    Try a MODESENSE6 command with 255 bytes of transfer length and
verify that we get residuals if the target returns less than the
requested amount of data.
    [SUCCESS] All Pages fetched.
    We got less than the requested 255 bytes from the target. Verify
that underflow is set.
    [SUCCESS] Residual underflow is set
passed

--Run Summary: Type      Total     Ran  Passed  Failed
               suites        1       1     n/a       0
               tests         1       1       1       0
               asserts       3       3       3       0
Tests completed with return value: 0



On Fri, May 31, 2013 at 4:50 AM, Alexander Nezhinsky
<nezhinsky at gmail.com> wrote:
> On Fri, May 31, 2013 at 8:30 AM, Ronnie Sahlberg
> <ronniesahlberg at gmail.com> wrote:
>
>> diff --git a/usr/spc.c b/usr/spc.c
>> index 074fdad..197e611 100644
>> --- a/usr/spc.c
>> +++ b/usr/spc.c
>> @@ -622,7 +622,7 @@ static int build_mode_page(uint8_t *data, struct mode_pg *pg,
>>   * Set a byte at the given index within dst buffer to val,
>>   * not exceeding dst_len bytes available at dst.
>>   */
>> -void set_byte_safe(uint8_t *dst, uint32_t dst_len, uint32_t index, int val)
>> +void set_byte_safe(uint8_t *dst, uint32_t index, uint32_t dst_len, int val)
>>  {
>>         if (index < dst_len)
>>                 dst[index] = (uint8_t)val;
>
> Oops, you are right! My fault, i swapped the arguments when calling
> the function.
>
>> @@ -713,10 +713,8 @@ int spc_mode_sense(int host_no, struct scsi_cmd *cmd)
>>                 set_byte_safe(data, 7, alloc_len, blk_desc_len & 0xff);
>>         }
>>
>> -       scsi_set_in_resid_by_actual(cmd, actual_len);
>>         return SAM_STAT_GOOD;
>>  sense:
>> -       scsi_set_in_resid_by_actual(cmd, 0);
>>         sense_data_build(cmd, key, asc);
>>         return SAM_STAT_CHECK_CONDITION;
>
> I guess we shouldn't remove these calls as the following reasoning:
>  > 2, These commands never return residual under/overflow so dont set
> the residuals for them
>  does not exactly apply here.
>
> Truncation of Data-IN buffer to ALLOC_LENGTH is not reflected in residual count,
> but if ALLOC_LENGTH is less than EDTL, it should produce a non-zero
> residual count.
> That is one of the purposes of scsi_set_in_resid_by_actual().
> Also, please refer to the commit log of d7af3dc1 which explains this logic.
> Is spc_mode_sense() really a special case which requires a different treatment?
>
> Alexander
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html