Re: [PATCH] Implement error checking in VERIFY10/12/16

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux SCSI]     [Linux RAID]     [Linux Clusters]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]

  Powered by Linux