[stgt] [PATCH] Implement error checking in VERIFY10/12/16
ronnie sahlberg
ronniesahlberg at gmail.com
Thu Jan 26 23:19:14 CET 2012
Tomo,
Please find an updated patch with your suggestions addressed.
On Fri, Jan 27, 2012 at 4:32 AM, FUJITA Tomonori
<fujita.tomonori at lab.ntt.co.jp> wrote:
> On Thu, 26 Jan 2012 16:29:11 +1100
> ronnie sahlberg <ronniesahlberg at gmail.com> wrote:
>
>> From 39ed0ca94bdc8ae4f90c72d56d47f962b3bc5ef5 Mon Sep 17 00:00:00 2001
>> From: Ronnie Sahlberg <ronniesahlberg at gmail.com>
>> Date: Thu, 26 Jan 2012 16:21:22 +1100
>> Subject: [PATCH] SBC VERIFY: implement VERIFY 10/12/16 and check the arguments
>>
>> TGTD does not support formatting protection information so make
>> TGTD fail a VERIFY command requesting vprotect != 0 with a proper
>> check condition.
>>
>> 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
>>
>> If BYTCHK==1 we must also verify the DATA-OUT buffer with the content of the media. Add a check that the data matches and return sense key:MISMATCH and the proper ASCQ if a mismatch is found.
>>
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg at gmail.com>
>> ---
>> usr/bs_rdwr.c | 27 +++++++++++++++++++++++++++
>> usr/sbc.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>> usr/scsi.c | 6 +++---
>> usr/scsi.h | 5 ++++-
>> 4 files changed, 85 insertions(+), 6 deletions(-)
>
> Thanks a lot! Looks great, some minor comments.
>
>> diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c
>> index acee73c..8d32f45 100644
>> --- a/usr/bs_rdwr.c
>> +++ b/usr/bs_rdwr.c
>> @@ -62,6 +62,7 @@ static void bs_rdwr_request(struct scsi_cmd *cmd)
>> int result = SAM_STAT_GOOD;
>> uint8_t key;
>> uint16_t asc;
>> + char *tmpbuf;
>>
>> ret = length = 0;
>> key = asc = 0;
>> @@ -121,6 +122,32 @@ static void bs_rdwr_request(struct scsi_cmd *cmd)
>> if (ret != 0)
>> set_medium_error(&result, &key, &asc);
>> break;
>> + case VERIFY_10:
>> + case VERIFY_12:
>> + case VERIFY_16:
>> + length = scsi_get_out_length(cmd);
>> +
>> + tmpbuf = malloc(length);
>> + if (tmpbuf == NULL) {
>
> Please use the following style instead?
>
> if (!tmpbuf)
>
>> + result = SAM_STAT_CHECK_CONDITION;
>> + key = HARDWARE_ERROR;
>> + asc = ASC_INTERNAL_TGT_FAILURE;
>> + break;
>> + }
>> +
>> + ret = pread64(fd, tmpbuf, length, cmd->offset);
>> +
>> + if (ret != length)
>> + set_medium_error(&result, &key, &asc);
>
> In this case, we need to avoid the following memcmp to avoid the error
> code overwriting (the memcmp fails in this case)?
>
> if (ret != length)
> set_medium_error(&result, &key, &asc);
> else if (memcmp(scsi_get_out_buffer(cmd), tmpbuf, length)) {
>
> ?
>
>> + if (memcmp(scsi_get_out_buffer(cmd), tmpbuf, length)) {
>> + result = SAM_STAT_CHECK_CONDITION;
>> + key = MISCOMPARE;
>> + asc = ASC_MISCOMPARE_DURING_VERIFY_OPERATION;
>> + }
>> +
>> + free(tmpbuf);
>> + break;
>> default:
>> break;
>> }
>> diff --git a/usr/sbc.c b/usr/sbc.c
>> index 53e785b..5805a72 100644
>> --- a/usr/sbc.c
>> +++ b/usr/sbc.c
>> @@ -265,7 +265,56 @@ sense:
>>
>> static int sbc_verify(int host_no, struct scsi_cmd *cmd)
>> {
>> + struct scsi_lu *lu = cmd->dev;
>> + unsigned char key;
>> + uint16_t asc;
>> + int vprotect, bytchk, ret;
>> + uint64_t lba;
>> + uint32_t tl;
>> +
>> + vprotect = cmd->scb[1] & 0xe0;
>> + if (vprotect != 0) {
>
> if (vprotect) {
>
>> + /* 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;
>> + }
>> +
>> + bytchk = cmd->scb[1] & 0x02;
>> + if (!bytchk) {
>> + /* no data compare with the media */
>> + return SAM_STAT_GOOD;
>> + }
>> +
>> + lba = scsi_rw_offset(cmd->scb) << cmd->dev->blk_shift;
>> + tl = scsi_rw_count(cmd->scb) << cmd->dev->blk_shift;
>> +
>> + /* Verify that we are not doing i/o beyond
>> + the end-of-lun */
>> + if (tl && (lba + tl > lu->size)) {
>> + key = ILLEGAL_REQUEST;
>> + asc = ASC_LBA_OUT_OF_RANGE;
>> + goto sense;
>> + }
>> +
>> + cmd->offset = lba;
>> + cmd->tl = tl;
>
> Use cmd->tl later?
>
>
>> + ret = cmd->dev->bst->bs_cmd_submit(cmd);
>> + if (ret) {
>> + key = HARDWARE_ERROR;
>> + asc = ASC_INTERNAL_TGT_FAILURE;
>> + goto sense;
>> + }
>> +
>> return SAM_STAT_GOOD;
>> +
>> +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 +555,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 +593,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},
>> }
>> diff --git a/usr/scsi.c b/usr/scsi.c
>> index 5f78bfd..7802ba1 100644
>> --- a/usr/scsi.c
>> +++ b/usr/scsi.c
>> @@ -117,7 +117,7 @@ uint64_t scsi_rw_offset(uint8_t *scb)
>> case READ_10:
>> case PRE_FETCH_10:
>> case WRITE_10:
>> - case VERIFY:
>> + case VERIFY_10:
>> case WRITE_VERIFY:
>> case SYNCHRONIZE_CACHE:
>> case READ_12:
>> @@ -160,7 +160,7 @@ uint32_t scsi_rw_count(uint8_t *scb)
>> case READ_10:
>> case PRE_FETCH_10:
>> case WRITE_10:
>> - case VERIFY:
>> + case VERIFY_10:
>> case WRITE_VERIFY:
>> case SYNCHRONIZE_CACHE:
>> cnt = (uint16_t)scb[7] << 8 | (uint16_t)scb[8];
>> @@ -261,7 +261,7 @@ int scsi_is_io_opcode(unsigned char op)
>> case WRITE_6:
>> case READ_10:
>> case WRITE_10:
>> - case VERIFY:
>> + case VERIFY_10:
>> case WRITE_VERIFY:
>> case READ_12:
>> case WRITE_12:
>> diff --git a/usr/scsi.h b/usr/scsi.h
>> index 0d33e32..ca1109a 100644
>> --- a/usr/scsi.h
>> +++ b/usr/scsi.h
>> @@ -40,7 +40,7 @@
>> #define SEEK_10 0x2b
>> #define POSITION_TO_ELEMENT 0x2b
>> #define WRITE_VERIFY 0x2e
>> -#define VERIFY 0x2f
>> +#define VERIFY_10 0x2f
>> #define SEARCH_HIGH 0x30
>> #define SEARCH_EQUAL 0x31
>> #define SEARCH_LOW 0x32
>> @@ -230,6 +230,9 @@
>> #define ASC_WRITE_PROTECT 0x2700
>> #define ASC_MEDIUM_OVERWRITE_ATTEMPTED 0x300c
>>
>> +/* Miscompare */
>> +#define ASC_MISCOMPARE_DURING_VERIFY_OPERATION 0x1d00
>> +
>>
>> /* PERSISTENT_RESERVE_IN service action codes */
>> #define PR_IN_READ_KEYS 0x00
>> --
>> 1.7.3.1
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SBC-VERIFY-implement-VERIFY-10-12-16-and-check-the-a.patch.gz
Type: application/x-gzip
Size: 2146 bytes
Desc: not available
URL: <http://lists.wpkg.org/pipermail/stgt/attachments/20120127/362944b8/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SBC-VERIFY-implement-VERIFY-10-12-16-and-check-the-a.patch
Type: text/x-diff
Size: 5434 bytes
Desc: not available
URL: <http://lists.wpkg.org/pipermail/stgt/attachments/20120127/362944b8/attachment-0002.patch>
More information about the stgt
mailing list