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

[Hannes Reinecke]

On 10/30/2014 11:38 AM, Paolo Bonzini wrote:
> 
> 
> 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:
> 
Okay.

> 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?
> 
No. I'm _fixing_ a bug.

scsi_disk.c:scsi_new_request() is just calling scsi_req_alloc(),
which does not set cmd.len.
Yet later on scsi_new_request() uses cmd.len to print out the CDB,
which at the time isn't initialized.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                   zSeries & Storage
address@hidden                        +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)