Hi Bart,
what do you think about the variant below instead which avoids
overloading the size variable?
---
>From 206696ec37cf4f6efe093964c2bdc96100de1f62 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@xxxxxx>
Date: Fri, 5 May 2017 11:40:38 +0200
Subject: target: fix and cleanup size calculation in sbc_parse_cdb
Calculate the data buffer size individually for each command instead of
trying to generalize it. This fixes the size calculation for VERIFY
and WRITE_VERIFY, while making the code a lot easier to understand.
Fixes: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing")
Reported-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Signed-off-by: Christoph Hellwig <hch@xxxxxx>
---
drivers/target/target_core_sbc.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 5807123214e5..0bd879b9ce38 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -899,12 +899,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
switch (cdb[0]) {
case READ_6:
sectors = transport_get_sectors_6(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_21(cdb);
cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
cmd->execute_cmd = sbc_execute_rw;
break;
case READ_10:
sectors = transport_get_sectors_10(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_32(cdb);
if (sbc_check_dpofua(dev, cmd, cdb))
@@ -919,6 +921,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
break;
case READ_12:
sectors = transport_get_sectors_12(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_32(cdb);
if (sbc_check_dpofua(dev, cmd, cdb))
@@ -933,6 +936,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
break;
case READ_16:
sectors = transport_get_sectors_16(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_64(cdb);
if (sbc_check_dpofua(dev, cmd, cdb))
@@ -947,12 +951,14 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
break;
case WRITE_6:
sectors = transport_get_sectors_6(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_21(cdb);
cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
cmd->execute_cmd = sbc_execute_rw;
break;
case WRITE_10:
sectors = transport_get_sectors_10(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_32(cdb);
if (sbc_check_dpofua(dev, cmd, cdb))
@@ -974,6 +980,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
goto check_lba;
case WRITE_12:
sectors = transport_get_sectors_12(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_32(cdb);
if (sbc_check_dpofua(dev, cmd, cdb))
@@ -988,6 +995,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
break;
case WRITE_16:
sectors = transport_get_sectors_16(cdb);
+ size = sbc_get_size(cmd, sectors);
cmd->t_task_lba = transport_lba_64(cdb);
if (sbc_check_dpofua(dev, cmd, cdb))
@@ -1005,6 +1013,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
!(cmd->se_cmd_flags & SCF_BIDI))
return TCM_INVALID_CDB_FIELD;
sectors = transport_get_sectors_10(cdb);
+ size = sbc_get_size(cmd, sectors);
if (sbc_check_dpofua(dev, cmd, cdb))
return TCM_INVALID_CDB_FIELD;
@@ -1024,6 +1033,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
switch (service_action) {
case XDWRITEREAD_32:
sectors = transport_get_sectors_32(cdb);
+ size = sbc_get_size(cmd, sectors);
if (sbc_check_dpofua(dev, cmd, cdb))
return TCM_INVALID_CDB_FIELD;
@@ -1116,7 +1126,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
sectors = transport_get_sectors_16(cdb);
cmd->t_task_lba = transport_lba_64(cdb);
}
+
+ /*
+ * XXX: why treat sectors / size check differently for
+ * the offload / non-offload cases?
+ */
if (ops->execute_sync_cache) {
+ size = sbc_get_size(cmd, sectors);
cmd->execute_cmd = ops->execute_sync_cache;
goto check_lba;
}
@@ -1205,9 +1221,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
end_lba, cmd->t_task_lba, sectors);
return TCM_ADDRESS_OUT_OF_RANGE;
}
-
- if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
- size = sbc_get_size(cmd, sectors);
}
return target_cmd_size_check(cmd, size);
--
2.11.0