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
h...@suse.de +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)
