[Qemu-devel] Re: [PATCH] scsi-generic: bugfixes for 'SCSIRequest' conversion

[Nicholas A. Bellinger]

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 <address@hidden>
> > 
> > This patch adds a handful of bugfixes for scsi-generic
> > that where added into:
> > 
> > commit a4194b3f79a85e111f000788ddec05d465748851
> > Author: Hannes Reinecke <address@hidden>
> > 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 address@hidden>
> > ---
> >  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