[stgt] [PATCH] Implement error checking in VERIFY10/12/16

[ronnie sahlberg]

To beat my own drum some more, and maybe encourage some block
enthusiasts to contribute, I have built two trivial tests for VERIFY10
to libiscsi :

Begin Advert ...

sahlberg at sahlberg-laptop:/data/sahlberg/libiscsi$ ./bin/iscsi-test
--test T0130_verify10_simple iscsi://127.0.0.1/iqn.ronnie.test/1
=========
Running test T0130_verify10_simple
Read+verify first 1-256 blocks ... [OK]
TEST T0130_verify10_simple [OK]

sahlberg at sahlberg-laptop:/data/sahlberg/libiscsi$ ./bin/iscsi-test
--test T0131_verify10_mismatch iscsi://127.0.0.1/iqn.ronnie.test/1
=========
Running test T0131_verify10_mismatch
Read+verify first 1-256 blocks ... [OK]
TEST T0131_verify10_mismatch [OK]



T0130 just tests that it reads 1-256 blocks at offset 0, then verifies
them again with VERIFY10.
Expected result is SAM_STATUS_GOOD for all calls.
T0131 reads 1-256 blocks at offset 0, then flips a few bits in one
randomly selected byte and then passes that buffer back in a VERIFY10
call.
Expected result is the target will respond with sense key==MISCOMPARE.


It is very easy to write such tests with the framework in libiscsi and
if we all work on it, maybe it develops into a good scsi compliance
testing tool.


This could be useful also for a "make test" for tgtd once/if we get
decent coverage of scsi opcodes.

End-Of-Advert ...

regards
ronnie sahlberg

On Thu, Jan 26, 2012 at 4:29 PM, ronnie sahlberg
<ronniesahlberg at gmail.com> wrote:
> Tomo, List
>
> Please find attached a patch for VERIFY 10/12/16
> It adds support for VERIFY12/16
>
> It also adds verification that the arguments passed from the initiator
> are correct (we do not support protection infomatiom)
> and return error for invalid requests.
>
> If BYTECHK == 1  we also verify the data with the content of the medium.
> If there is a mismatch we return check-condition with key==MISCOMPARE
> and the proper ASCQ
>
>
>
> Please also find a small capture file showing TGTD returning proper
> MISCOMPARE when a VERIFY10 request comes in with bad data (one byte
> modified in one of the blocks)
>
>
> To test VERIFY10 failure I created VERIFY10 test cases for the
> libiscsi test tool to verify that a target responds GOOD when VERIFY10
> matches the data on the medium and that the target responds MISMATCH
> if the data is known to be bad.
>
>
>
> regards
> ronnie sahlberg
>
>
> On Thu, Jan 26, 2012 at 12:00 PM, FUJITA Tomonori
> <fujita.tomonori at lab.ntt.co.jp> wrote:
>> On Thu, 26 Jan 2012 11:17:39 +1100
>> ronnie sahlberg <ronniesahlberg at gmail.com> wrote:
>>
>>> From dfb19f35441d137ef4b82cec924a8746cb1c040d Mon Sep 17 00:00:00 2001
>>> From: Ronnie Sahlberg <ronniesahlberg at gmail.com>
>>> Date: Thu, 26 Jan 2012 10:35:22 +1100
>>> Subject: [PATCH] SBC VERIFY: Update VERIFY to check vprotect argument
>>>
>>> TGTD does not support formatting protection information so make
>>> TGTD fail a VERIFY command requesting vprotect != 0 with a proper
>>> check condition.
>>>
>>> Add VERIFY12/VERIFY16. These are both identical to the already existing VERIFY10 as far as we are converned.
>>>
>>> See SBC 5.22 VERIFY 10 COMMAND,  tables 65 and 66
>>> If the media is not formatted with protection information (as in TGTD)
>>> any value of vprotect other than 000b is an error condition and the device h
>>>
>>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg at gmail.com>
>>> ---
>>>  usr/sbc.c |   25 ++++++++++++++++++++++---
>>>  1 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/usr/sbc.c b/usr/sbc.c
>>> index 53e785b..23b1778 100644
>>> --- a/usr/sbc.c
>>> +++ b/usr/sbc.c
>>> @@ -265,7 +265,26 @@ sense:
>>>
>>>  static int sbc_verify(int host_no, struct scsi_cmd *cmd)
>>>  {
>>> -     return SAM_STAT_GOOD;
>>> +     unsigned char key;
>>> +     uint16_t asc;
>>> +     int vprotect;
>>> +
>>> +     vprotect = cmd->scb[1] & 0xe0;
>>> +     if (vprotect != 0) {
>>> +             /* we dont support formatting with protection information,
>>> +              * so all verify with vprotect!=0 is an error condition
>>> +              */
>>> +             key = ILLEGAL_REQUEST;
>>> +             asc = ASC_INVALID_FIELD_IN_CDB;
>>> +             goto sense;
>>> +     }
>>> +
>>> +     return SAM_STAT_GOOD;
>>
>> Hmm, should we implement VERIFY (such as data comparison) instead of
>> returning GOOD if we replace illegal_op?
>>
>>
>>> +sense:
>>> +     scsi_set_in_resid_by_actual(cmd, 0);
>>> +     sense_data_build(cmd, key, asc);
>>> +     return SAM_STAT_CHECK_CONDITION;
>>>  }
>>>
>>>  static int sbc_service_action(int host_no, struct scsi_cmd *cmd)
>>> @@ -506,7 +525,7 @@ static struct device_type_template sbc_template = {
>>>               {spc_illegal_op,},
>>>               {spc_illegal_op,},
>>>               {spc_illegal_op,},
>>> -             {spc_illegal_op,},
>>> +             {sbc_verify, NULL, PR_EA_FA|PR_EA_FN},
>>>
>>>               /* 0x90 */
>>>               {sbc_rw, NULL, PR_EA_FA|PR_EA_FN}, /*PRE_FETCH_16 */
>>> @@ -544,7 +563,7 @@ static struct device_type_template sbc_template = {
>>>               {spc_illegal_op,},
>>>               {spc_illegal_op,},
>>>               {spc_illegal_op,},
>>> -             {spc_illegal_op,},
>>> +             {sbc_verify, NULL, PR_EA_FA|PR_EA_FN},
>>>
>>>               [0xb0 ... 0xff] = {spc_illegal_op},
>>>       }
>>> --
>>> 1.7.3.1
>>>
--
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