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

[Hannes Reinecke <hare@suse.de>]

On 11/24/2010 09:18 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch adds a handful of bugfixes for scsi-generic
> that where added into:
> 
> commit a4194b3f79a85e111f000788ddec05d465748851
> Author: Hannes Reinecke <hare@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 nab@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.  */

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)