Re: [Qemu-devel] [PATCH][v2] scsi-generic: correct error management

[Aurelien Jarno]

On Mon, Oct 13, 2008 at 05:09:57PM +0200, Laurent Vivier wrote:
> Hi,
> 
> this patch allows to fully use a tape device connected to qemu through
> the scsi-generic interface.
> 
> Previous patch introduced tape SCSI commands management, this one
> improve error case management:
> 
> - the SCSI controller command completion must be called with the status
> value, not the sense value. In the case of scsi-generic, the SCSI status
> is given by the field status of sg_io_hdr_t (the value is left shifted
> by one regarding status codes defined in /usr/include/scsi/scsi.h)
> 
> - when a read is aborted due to a mark/EOF/EOD/EOM, the len reported to
> controller can be 0. LSI controller emulation doesn't know how to manage
> this. A workaround found is to call the completion routine with
> SCSI_REASON_DONE just after calling it with SCSI_REASON_DATA with len=0.
> 
> This patch also manages correctly the block size of the tape device.
> 
> This patch has been tested with a real tape device "HP C5683A", linux
> guest (debian etch) and tools like "mt", "tar" and "btape".
> 
> Windows guest is not better supported than before...
> 
> Changelog:
> 
> v2: Improve management of sense buffer. Store sense buffer length and
> generate a fake sense buffer when an invalid LUN is given. Allow
> "REQUEST SENSE" on an invalid LUN.
> 

Applied, thanks.

> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  hw/scsi-generic.c |  132 
> ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 103 insertions(+), 29 deletions(-)
> 
> Index: qemu/hw/scsi-generic.c
> ===================================================================
> --- qemu.orig/hw/scsi-generic.c       2008-10-13 13:37:09.000000000 +0200
> +++ qemu/hw/scsi-generic.c    2008-10-13 16:35:34.000000000 +0200
> @@ -84,6 +84,7 @@ struct SCSIDeviceState
>      void *opaque;
>      int driver_status;
>      uint8_t sensebuf[SCSI_SENSE_BUF_SIZE];
> +    uint8_t senselen;
>  };
>  
>  /* Global pool of SCSIRequest structures.  */
> @@ -154,25 +155,30 @@ static void scsi_command_complete(void *
>      SCSIRequest *r = (SCSIRequest *)opaque;
>      SCSIDeviceState *s = r->dev;
>      uint32_t tag;
> -    int sense;
> +    int status;
>  
>      s->driver_status = r->io_header.driver_status;
> +    if (s->driver_status & SG_ERR_DRIVER_SENSE)
> +        s->senselen = r->io_header.sb_len_wr;
> +
>      if (ret != 0)
> -        sense = HARDWARE_ERROR;
> +        status = BUSY << 1;
>      else {
>          if (s->driver_status & SG_ERR_DRIVER_TIMEOUT) {
> -            sense = HARDWARE_ERROR;
> +            status = BUSY << 1;
>              BADF("Driver Timeout\n");
> -        } else if ((s->driver_status & SG_ERR_DRIVER_SENSE) == 0)
> -            sense = NO_SENSE;
> +        } else if (r->io_header.status)
> +            status = r->io_header.status;
> +        else if (s->driver_status & SG_ERR_DRIVER_SENSE)
> +            status = CHECK_CONDITION << 1;
>          else
> -            sense = s->sensebuf[2];
> +            status = GOOD << 1;
>      }
> -
> -    DPRINTF("Command complete 0x%p tag=0x%x sense=%d\n", r, r->tag, sense);
> +    DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
> +            r, r->tag, status);
>      tag = r->tag;
>      scsi_remove_request(r);
> -    s->completion(s->opaque, SCSI_REASON_DONE, tag, sense);
> +    s->completion(s->opaque, SCSI_REASON_DONE, tag, status);
>  }
>  
>  /* Cancel a pending data transfer.  */
> @@ -251,6 +257,8 @@ static void scsi_read_complete(void * op
>  
>      r->len = -1;
>      s->completion(s->opaque, SCSI_REASON_DATA, r->tag, len);
> +    if (len == 0)
> +        scsi_command_complete(r, 0);
>  }
>  
>  /* Read more data from scsi device into buffer.  */
> @@ -276,14 +284,17 @@ static void scsi_read_data(SCSIDevice *d
>  
>      if (r->cmd[0] == REQUEST_SENSE && s->driver_status & SG_ERR_DRIVER_SENSE)
>      {
> -        int len = MIN(r->len, SCSI_SENSE_BUF_SIZE);
> -        memcpy(r->buf, s->sensebuf, len);
> +        s->senselen = MIN(r->len, s->senselen);
> +        memcpy(r->buf, s->sensebuf, s->senselen);
>          r->io_header.driver_status = 0;
> +        r->io_header.status = 0;
> +        r->io_header.dxfer_len  = s->senselen;
>          r->len = -1;
> +        DPRINTF("Data ready tag=0x%x len=%d\n", r->tag, s->senselen);
>          DPRINTF("Sense: %d %d %d %d %d %d %d %d\n",
>                  r->buf[0], r->buf[1], r->buf[2], r->buf[3],
>                  r->buf[4], r->buf[5], r->buf[6], r->buf[7]);
> -        s->completion(s->opaque, SCSI_REASON_DATA, r->tag, len);
> +        s->completion(s->opaque, SCSI_REASON_DATA, r->tag, s->senselen);
>          return;
>      }
>  
> @@ -305,6 +316,12 @@ static void scsi_write_complete(void * o
>          return;
>      }
>  
> +    if (r->cmd[0] == MODE_SELECT && r->cmd[4] == 12 &&
> +        r->dev->type == TYPE_TAPE) {
> +        r->dev->blocksize = (r->buf[9] << 16) | (r->buf[10] << 8) | 
> r->buf[11];
> +        DPRINTF("block size %d\n", r->dev->blocksize);
> +    }
> +
>      scsi_command_complete(r, ret);
>  }
>  
> @@ -437,6 +454,9 @@ static int scsi_length(uint8_t *cmd, int
>      case READ_12:
>          *len *= blocksize;
>          break;
> +    case INQUIRY:
> +        *len = cmd[4] | (cmd[3] << 8);
> +        break;
>      }
>      return 0;
>  }
> @@ -519,15 +539,6 @@ static int32_t scsi_send_command(SCSIDev
>      SCSIRequest *r;
>      int ret;
>  
> -    /* ??? Tags are not unique for different luns.  We only implement a
> -       single lun, so this should not matter.  */
> -
> -    if (lun != s->lun || (cmd[1] >> 5) != s->lun) {
> -        DPRINTF("Unimplemented LUN %d\n", lun ? lun : cmd[1] >> 5);
> -        s->completion(s->opaque, SCSI_REASON_DONE, tag, ILLEGAL_REQUEST);
> -        return 0;
> -    }
> -
>      if (s->type == TYPE_TAPE) {
>          if (scsi_stream_length(cmd, s->blocksize, &cmdlen, &len) == -1) {
>              BADF("Unsupported command length, command %x\n", cmd[0]);
> @@ -543,6 +554,23 @@ static int32_t scsi_send_command(SCSIDev
>      DPRINTF("Command: lun=%d tag=0x%x data=0x%02x len %d\n", lun, tag,
>              cmd[0], len);
>  
> +    if (cmd[0] != REQUEST_SENSE &&
> +        (lun != s->lun || (cmd[1] >> 5) != s->lun)) {
> +        DPRINTF("Unimplemented LUN %d\n", lun ? lun : cmd[1] >> 5);
> +
> +        s->sensebuf[0] = 0x70;
> +        s->sensebuf[1] = 0x00;
> +        s->sensebuf[2] = ILLEGAL_REQUEST;
> +        s->sensebuf[3] = 0x00;
> +        s->sensebuf[4] = 0x00;
> +        s->sensebuf[5] = 0x00;
> +        s->sensebuf[6] = 0x00;
> +        s->senselen = 7;
> +        s->driver_status = SG_ERR_DRIVER_SENSE;
> +        s->completion(s->opaque, SCSI_REASON_DONE, tag, CHECK_CONDITION << 
> 1);
> +        return 0;
> +    }
> +
>      r = scsi_find_request(s, tag);
>      if (r) {
>          BADF("Tag 0x%x already in use %p\n", tag, r);
> @@ -619,6 +647,43 @@ static int get_blocksize(BlockDriverStat
>      return (buf[4] << 24) | (buf[5] << 16) | (buf[6] << 8) | buf[7];
>  }
>  
> +static int get_stream_blocksize(BlockDriverState *bdrv)
> +{
> +    uint8_t cmd[6];
> +    uint8_t buf[12];
> +    uint8_t sensebuf[8];
> +    sg_io_hdr_t io_header;
> +    int ret;
> +
> +    memset(cmd, 0, sizeof(cmd));
> +    memset(buf, 0, sizeof(buf));
> +    cmd[0] = MODE_SENSE;
> +    cmd[4] = sizeof(buf);
> +
> +    memset(&io_header, 0, sizeof(io_header));
> +    io_header.interface_id = 'S';
> +    io_header.dxfer_direction = SG_DXFER_FROM_DEV;
> +    io_header.dxfer_len = sizeof(buf);
> +    io_header.dxferp = buf;
> +    io_header.cmdp = cmd;
> +    io_header.cmd_len = sizeof(cmd);
> +    io_header.mx_sb_len = sizeof(sensebuf);
> +    io_header.sbp = sensebuf;
> +    io_header.timeout = 6000; /* XXX */
> +
> +    ret = bdrv_pwrite(bdrv, -1, &io_header, sizeof(io_header));
> +    if (ret == -1)
> +        return -1;
> +
> +    while ((ret = bdrv_pread(bdrv, -1, &io_header, sizeof(io_header))) == -1 
> &&
> +           errno == EINTR);
> +
> +    if (ret == -1)
> +        return -1;
> +
> +    return (buf[9] << 16) | (buf[10] << 8) | buf[11];
> +}
> +
>  static void scsi_destroy(SCSIDevice *d)
>  {
>      SCSIRequest *r, *n;
> @@ -673,17 +738,26 @@ SCSIDevice *scsi_generic_init(BlockDrive
>      s->completion = completion;
>      s->opaque = opaque;
>      s->lun = scsiid.lun;
> +    DPRINTF("LUN %d\n", s->lun);
>      s->type = scsiid.scsi_type;
> -    s->blocksize = get_blocksize(s->bdrv);
> +    DPRINTF("device type %d\n", s->type);
> +    if (s->type == TYPE_TAPE) {
> +        s->blocksize = get_stream_blocksize(s->bdrv);
> +        if (s->blocksize == -1)
> +            s->blocksize = 0;
> +    } else {
> +        s->blocksize = get_blocksize(s->bdrv);
> +        /* removable media returns 0 if not present */
> +        if (s->blocksize <= 0) {
> +            if (s->type == TYPE_ROM || s->type  == TYPE_WORM)
> +                s->blocksize = 2048;
> +            else
> +                s->blocksize = 512;
> +        }
> +    }
> +    DPRINTF("block size %d\n", s->blocksize);
>      s->driver_status = 0;
>      memset(s->sensebuf, 0, sizeof(s->sensebuf));
> -    /* removable media returns 0 if not present */
> -    if (s->blocksize <= 0) {
> -        if (s->type == TYPE_ROM || s->type  == TYPE_WORM)
> -            s->blocksize = 2048;
> -        else
> -            s->blocksize = 512;
> -    }
>  
>      /* define function to manage device */
>  


-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net