Am 28.10.2010 11:12, schrieb Stefan Hajnoczi:
> On Mon, Oct 25, 2010 at 05:17:33PM +0200, Kevin Wolf wrote:
>> This implements the rerror option for SCSI disks.
>>
>> Signed-off-by: Kevin Wolf <kw...@redhat.com>
>> ---
>> blockdev.c | 2 +-
>> hw/scsi-disk.c | 91
>> ++++++++++++++++++++++++++++++++++++++------------------
>> 2 files changed, 63 insertions(+), 30 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index ff7602b..160fa84 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -326,7 +326,7 @@ DriveInfo *drive_init(QemuOpts *opts, int
>> default_to_scsi, int *fatal_error)
>>
>> on_read_error = BLOCK_ERR_REPORT;
>> if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
>> - if (type != IF_IDE && type != IF_VIRTIO && type != IF_NONE) {
>> + if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type
>> != IF_NONE) {
>> fprintf(stderr, "rerror is no supported by this format\n");
>
> This is a good opportunity to fix the "is *no* supported" typo in the error
> message.
>> return NULL;
>> }
>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
>> index 9628b39..55ba558 100644
>> --- a/hw/scsi-disk.c
>> +++ b/hw/scsi-disk.c
>> @@ -41,7 +41,10 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__);
>> } while (0)
>> #define SCSI_DMA_BUF_SIZE 131072
>> #define SCSI_MAX_INQUIRY_LEN 256
>>
>> -#define SCSI_REQ_STATUS_RETRY 0x01
>> +#define SCSI_REQ_STATUS_RETRY 0x01
>> +#define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
>> +#define SCSI_REQ_STATUS_RETRY_READ 0x00
>> +#define SCSI_REQ_STATUS_RETRY_WRITE 0x02
>>
>> typedef struct SCSIDiskState SCSIDiskState;
>>
>> @@ -70,6 +73,8 @@ struct SCSIDiskState
>> char *serial;
>> };
>>
>> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
>> +
>> static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
>> uint32_t lun)
>> {
>> @@ -127,34 +132,30 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
>> static void scsi_read_complete(void * opaque, int ret)
>> {
>> SCSIDiskReq *r = (SCSIDiskReq *)opaque;
>> + int n;
>>
>> r->req.aiocb = NULL;
>>
>> if (ret) {
>> - DPRINTF("IO error\n");
>> - r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
>> - scsi_command_complete(r, CHECK_CONDITION, NO_SENSE);
>> - return;
>> + if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
>> + return;
>> + }
>> }
>> +
>> DPRINTF("Data ready tag=0x%x len=%zd\n", r->req.tag, r->iov.iov_len);
>>
>> + n = r->iov.iov_len / 512;
>> + r->sector += n;
>> + r->sector_count -= n;
>> r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag,
>> r->iov.iov_len);
>> }
>>
>> -/* Read more data from scsi device into buffer. */
>> -static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>> +
>> +static void scsi_read_request(SCSIDiskReq *r)
>> {
>> - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
>> - SCSIDiskReq *r;
>> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> uint32_t n;
>>
>> - r = scsi_find_request(s, tag);
>> - if (!r) {
>> - BADF("Bad read tag 0x%x\n", tag);
>> - /* ??? This is the wrong error. */
>> - scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
>> - return;
>> - }
>> if (r->sector_count == (uint32_t)-1) {
>> DPRINTF("Read buf_len=%zd\n", r->iov.iov_len);
>> r->sector_count = 0;
>> @@ -177,14 +178,34 @@ static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>> scsi_read_complete, r);
>> if (r->req.aiocb == NULL)
>> scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
>> - r->sector += n;
>> - r->sector_count -= n;
>> }
>>
>> -static int scsi_handle_write_error(SCSIDiskReq *r, int error)
>> +/* Read more data from scsi device into buffer. */
>> +static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>> {
>> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
>> + SCSIDiskReq *r;
>> +
>> + r = scsi_find_request(s, tag);
>> + if (!r) {
>> + BADF("Bad read tag 0x%x\n", tag);
>> + /* ??? This is the wrong error. */
>> + scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
>> + return;
>> + }
>> +
>> + if (r->req.aiocb) {
>> + BADF("Data transfer already in progress\n");
>> + }
>
> Can this be triggered from the guest? If yes, then we need to (optionally)
> cancel the request with an error and then return. If no, then this should be
> an assert.
Honestly, I don't know for sure. It's just the same as what we're doing
in scsi_write_disk.
But after a look at the callers, I think an assert should work.
>> +
>> + scsi_read_request(r);
>> +}
>> +
>> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
>> +{
>> + int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
>> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> - BlockErrorAction action = bdrv_get_on_error(s->bs, 0);
>> + BlockErrorAction action = bdrv_get_on_error(s->bs, is_read);
>>
>> if (action == BLOCK_ERR_IGNORE) {
>> bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);
>> @@ -193,10 +214,16 @@ static int scsi_handle_write_error(SCSIDiskReq *r, int
>> error)
>>
>> if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>> || action == BLOCK_ERR_STOP_ANY) {
>> - r->status |= SCSI_REQ_STATUS_RETRY;
>> +
>> + type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
>> + r->status |= SCSI_REQ_STATUS_RETRY | type;
>> +
>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
>> vm_stop(0);
>> } else {
>> + if (type == SCSI_REQ_STATUS_RETRY_READ) {
>> + r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag,
>> 0);
>> + }
>> scsi_command_complete(r, CHECK_CONDITION,
>> HARDWARE_ERROR);
>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, 0);
>
> We don't report whether this was a read or a write here?
Whoops, good catch.
Kevin
