Message ID | 54521520.4000805@redhat.com |
---|---|
State | New |
Headers | show |
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
On 10/30/2014 12:26 PM, Hannes Reinecke wrote: > > Why would req->cmd.len be wrong though? Are you masking another bug? > > No. I'm _fixing_ a bug. Sure, I just wasn't sure which/how. :) > 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. Ah, right---req->cmd is set after scsi_new_request() returns. Paolo
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; }