On Wed, 2010-11-24 at 09:57 +0100, Hannes Reinecke wrote:
> On 11/24/2010 09:18 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <n...@linux-iscsi.org>
> >
> > This patch adds a handful of bugfixes for scsi-generic
> > that where added into:
> >
> > commit a4194b3f79a85e111f000788ddec05d465748851
> > Author: Hannes Reinecke <h...@suse.de>
> > Date: Mon Nov 22 15:39:33 2010 -0800
> >
> > scsi: Use 'SCSIRequest' directly
> >
> > this includes:
> >
> > *) Fix incorrect errno usage in switch() statement within
> > scsi_command_complete()
> >
> > *) Remove bogus scsi_command_complete() for residual case
> > within scsi_read_complete()
> >
> > *) Remove incorrect '-' sign from return in scsi_send_command()
> >
> > Tested with .37-rc2 TCM_Loop FILEIO backstores on KVM host into
> > Debian Lenny v2.6.26 KVM guest with an xfs filesystem mount.
> >
> > Signed-off-by: Nicholas A. Bellinger n...@linux-iscsi.org>
> > ---
> > hw/scsi-generic.c | 14 ++++++--------
> > 1 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
> > index 7d30115..dc277cc 100644
> > --- a/hw/scsi-generic.c
> > +++ b/hw/scsi-generic.c
> > @@ -146,13 +146,13 @@ static void scsi_command_complete(void *opaque, int
> > ret)
> >
> > if (ret != 0) {
> > switch(ret) {
> > - case ENODEV:
> > + case -ENODEV:
> > s->senselen = scsi_set_sense(s,
> > SENSE_CODE(LUN_NOT_SUPPORTED));
> > break;
> > - case EINVAL:
> > + case -EINVAL:
> > s->senselen = scsi_set_sense(s, SENSE_CODE(INVALID_FIELD));
> > break;
> > - case EBADR:
> > + case -EBADR:
> > s->senselen = scsi_set_sense(s,
> > SENSE_CODE(TARGET_FAILURE));
> > break;
> > default:
> Oh. Correct. Although we could do a 'switch (-ret)' here.
>
> > @@ -230,12 +230,10 @@ static void scsi_read_complete(void * opaque, int ret)
> > return;
> > }
> > len = r->io_header.dxfer_len - r->io_header.resid;
> > - DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
> > + DPRINTF("Data ready tag=0x%x remaining len=%d\n", r->req.tag, len);
> >
> > r->len = -1;
> > r->req.bus->complete(&r->req, SCSI_REASON_DATA, len);
> > - if (len == 0)
> > - scsi_command_complete(r, 0);
> > }
> >
> > /* Read more data from scsi device into buffer. */
>
> Yes, that's correct (after a fashion).
> Difference is that we're having to do one more callback into
> scsi_read_data() with this hunk. But ok, ack.
>
> > @@ -375,7 +373,7 @@ static int32_t scsi_send_command(SCSIRequest *req,
> > uint8_t *cmd)
> > }
> > scsi_req_fixup(&r->req);
> >
> > - DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", lun, tag,
> > + DPRINTF("Command: lun=%d tag=0x%x len %zd data=0x%02x", req->lun,
> > req->tag,
> > r->req.cmd.xfer, cmd[0]);
> >
> > #ifdef DEBUG_SCSI
> > @@ -414,7 +412,7 @@ static int32_t scsi_send_command(SCSIRequest *req,
> > uint8_t *cmd)
> > r->len = r->req.cmd.xfer;
> > if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
> > r->len = 0;
> > - return -r->req.cmd.xfer;
> > + return r->req.cmd.xfer;
> > }
> >
> > return r->req.cmd.xfer;
> NACK.
> Returning a negative value here means we're about to execute a write.
> Cf comment at the start of the function:
>
> /* Execute a scsi command. Returns the length of the data
> expected by the command. This will be Positive for data
> transfers from the device (eg. disk reads), negative for
> transfers to the device (eg. disk writes),
> and zero if the command does not transfer any data. */
>
OK, changint this back in megasas-upstream-v1 here along with the same
bug that was added to the the outgoing hw/scsi-bsg.c..
But yeah, this is a really confusing interface and seems like it really
should be fixed, right..? Unless there is some legacy reason that it
makes it difficult to do so..?
--nab
