Re: [Qemu-devel] [PATCH 03/17] scsi: Rename scsi_cdb_length() to scsi_xfer_length()

[Paolo Bonzini]

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 <address@hidden>
> ---
>  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);
>