On 10/29/2014 08:53 AM, Hannes Reinecke wrote:
> scsi_cdb_length() does not return the length of the cdb, but
> the transfersize encoded in the cdb. So rename it to scsi_xfer_length()
> and add a new scsi_cdb_length() which actually does return the
> length of the cdb.
This makes sense, but it messes up the function names even more. We now
have ata_passthrough_*_xfer_size, scsi_xfer_length, scsi_req_length,
scsi_req_*_length. Let's standardize on scsi_*_xfer:
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7763847..3eccb1b 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -820,7 +820,7 @@ static int ata_passthrough_xfer_unit(SCSIDevice *dev,
uint8_t *buf)
return xfer_unit;
}
-static int ata_passthrough_12_xfer_size(SCSIDevice *dev, uint8_t *buf)
+static int ata_passthrough_12_xfer(SCSIDevice *dev, uint8_t *buf)
{
int length = buf[2] & 0x3;
int xfer;
@@ -843,7 +843,7 @@ static int ata_passthrough_12_xfer_size(SCSIDevice *dev,
uint8_t *buf)
return xfer * unit;
}
-static int ata_passthrough_16_xfer_size(SCSIDevice *dev, uint8_t *buf)
+static int ata_passthrough_16_xfer(SCSIDevice *dev, uint8_t *buf)
{
int extend = buf[1] & 0x1;
int length = buf[2] & 0x3;
@@ -874,11 +874,11 @@ uint32_t scsi_data_cdb_length(uint8_t *buf)
if ((buf[0] >> 5) == 0 && buf[4] == 0) {
return 256;
} else {
- return scsi_xfer_length(buf);
+ return scsi_cdb_xfer(buf);
}
}
-uint32_t scsi_xfer_length(uint8_t *buf)
+uint32_t scsi_cdb_xfer(uint8_t *buf)
{
switch (buf[0] >> 5) {
case 0:
@@ -899,9 +899,9 @@ uint32_t scsi_xfer_length(uint8_t *buf)
}
}
-static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
{
- cmd->xfer = scsi_xfer_length(buf);
+ cmd->xfer = scsi_cdb_xfer(buf);
switch (buf[0]) {
case TEST_UNIT_READY:
case REWIND:
@@ -1032,17 +1032,17 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice
*dev, uint8_t *buf)
/* BLANK command of MMC */
cmd->xfer = 0;
} else {
- cmd->xfer = ata_passthrough_12_xfer_size(dev, buf);
+ cmd->xfer = ata_passthrough_12_xfer(dev, buf);
}
break;
case ATA_PASSTHROUGH_16:
- cmd->xfer = ata_passthrough_16_xfer_size(dev, buf);
+ cmd->xfer = ata_passthrough_16_xfer(dev, buf);
break;
}
return 0;
}
-static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t
*buf)
+static int scsi_req_stream_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t
*buf)
{
switch (buf[0]) {
/* stream commands */
@@ -1097,12 +1097,12 @@ static int scsi_req_stream_length(SCSICommand *cmd,
SCSIDevice *dev, uint8_t *bu
break;
/* generic commands */
default:
- return scsi_req_length(cmd, dev, buf);
+ return scsi_req_xfer(cmd, dev, buf);
}
return 0;
}
-static int scsi_req_medium_changer_length(SCSICommand *cmd, SCSIDevice *dev,
uint8_t *buf)
+static int scsi_req_medium_changer_xfer(SCSICommand *cmd, SCSIDevice *dev,
uint8_t *buf)
{
switch (buf[0]) {
/* medium changer commands */
@@ -1119,7 +1119,7 @@ static int scsi_req_medium_changer_length(SCSICommand
*cmd, SCSIDevice *dev, uin
/* generic commands */
default:
- return scsi_req_length(cmd, dev, buf);
+ return scsi_req_xfer(cmd, dev, buf);
}
return 0;
}
@@ -1240,13 +1240,13 @@ int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand
*cmd, uint8_t *buf)
switch (dev->type) {
case TYPE_TAPE:
- rc = scsi_req_stream_length(cmd, dev, buf);
+ rc = scsi_req_stream_xfer(cmd, dev, buf);
break;
case TYPE_MEDIUM_CHANGER:
- rc = scsi_req_medium_changer_length(cmd, dev, buf);
+ rc = scsi_req_medium_changer_xfer(cmd, dev, buf);
break;
default:
- rc = scsi_req_length(cmd, dev, buf);
+ rc = scsi_req_xfer(cmd, dev, buf);
break;
}
> With that DEBUG_SCSI can now display the correct CDB buffer.
Why would req->cmd.len be wrong though? Are you masking another bug?
Paolo
> Signed-off-by: Hannes Reinecke <h...@suse.de>
> ---
> hw/scsi/scsi-bus.c | 31 +++++++++++++++++++------------
> hw/scsi/scsi-disk.c | 2 +-
> include/hw/scsi/scsi.h | 3 ++-
> 3 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 022a524..919a86c 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -879,11 +879,11 @@ uint32_t scsi_data_cdb_length(uint8_t *buf)
> if ((buf[0] >> 5) == 0 && buf[4] == 0) {
> return 256;
> } else {
> - return scsi_cdb_length(buf);
> + return scsi_xfer_length(buf);
> }
> }
>
> -uint32_t scsi_cdb_length(uint8_t *buf)
> +uint32_t scsi_xfer_length(uint8_t *buf)
> {
> switch (buf[0] >> 5) {
> case 0:
> @@ -906,7 +906,7 @@ uint32_t scsi_cdb_length(uint8_t *buf)
>
> static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
> {
> - cmd->xfer = scsi_cdb_length(buf);
> + cmd->xfer = scsi_xfer_length(buf);
> switch (buf[0]) {
> case TEST_UNIT_READY:
> case REWIND:
> @@ -1213,28 +1213,35 @@ static uint64_t scsi_cmd_lba(SCSICommand *cmd)
> return lba;
> }
>
> -int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
> -{
> - int rc;
> +int scsi_cdb_length(uint8_t *buf) {
> + int cdb_len;
>
> - cmd->lba = -1;
> switch (buf[0] >> 5) {
> case 0:
> - cmd->len = 6;
> + cdb_len = 6;
> break;
> case 1:
> case 2:
> - cmd->len = 10;
> + cdb_len = 10;
> break;
> case 4:
> - cmd->len = 16;
> + cdb_len = 16;
> break;
> case 5:
> - cmd->len = 12;
> + cdb_len = 12;
> break;
> default:
> - return -1;
> + cdb_len = -1;
> }
> + return cdb_len;
> +}
> +
> +int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
> +{
> + int rc;
> +
> + cmd->lba = -1;
> + cmd->len = scsi_cdb_length(buf);
>
> switch (dev->type) {
> case TYPE_TAPE:
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index ae9e08d..30e3789 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2393,7 +2393,7 @@ static SCSIRequest *scsi_new_request(SCSIDevice *d,
> uint32_t tag, uint32_t lun,
> DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]);
> {
> int i;
> - for (i = 1; i < req->cmd.len; i++) {
> + for (i = 1; i < scsi_cdb_length(buf); i++) {
> printf(" 0x%02x", buf[i]);
> }
> printf("\n");
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index caaa320..4e9bbd1 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -240,7 +240,8 @@ extern const struct SCSISense
> sense_code_SPACE_ALLOC_FAILED;
> #define SENSE_CODE(x) sense_code_ ## x
>
> uint32_t scsi_data_cdb_length(uint8_t *buf);
> -uint32_t scsi_cdb_length(uint8_t *buf);
> +uint32_t scsi_xfer_length(uint8_t *buf);
> +int scsi_cdb_length(uint8_t *buf);
> int scsi_sense_valid(SCSISense sense);
> int scsi_build_sense(uint8_t *in_buf, int in_len,
> uint8_t *buf, int len, bool fixed);
>
