Tomo,
Please find an updated patch with your suggestions addressed.
On Fri, Jan 27, 2012 at 4:32 AM, FUJITA Tomonori
<fujita.tomonori@xxxxxxxxxxxxx> wrote:
> On Thu, 26 Jan 2012 16:29:11 +1100
> ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
>
>> From 39ed0ca94bdc8ae4f90c72d56d47f962b3bc5ef5 Mon Sep 17 00:00:00 2001
>> From: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
>> 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@xxxxxxxxx>
>> ---
>> 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
>>
Attachment:
0001-SBC-VERIFY-implement-VERIFY-10-12-16-and-check-the-a.patch.gz
Description: GNU Zip compressed data
From 697c5dacd9897b3b1b0bf2d8d70956d930f2d1a9 Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
Date: Fri, 27 Jan 2012 09:18:00 +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.
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@xxxxxxxxx>
---
usr/bs_rdwr.c | 26 ++++++++++++++++++++++++++
usr/sbc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
usr/scsi.c | 6 +++---
usr/scsi.h | 5 ++++-
4 files changed, 83 insertions(+), 6 deletions(-)
diff --git a/usr/bs_rdwr.c b/usr/bs_rdwr.c
index acee73c..c6f6845 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,31 @@ 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) {
+ 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);
+ else 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..d483732 100644
--- a/usr/sbc.c
+++ b/usr/sbc.c
@@ -265,7 +265,55 @@ 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) {
+ /* 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;
+
+ 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 +554,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 +592,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
