[Qemu-devel] [PULL 00/10] Block patches

[Qemu-devel] [PULL 00/10] Block patches

[Kevin Wolf]

The following changes since commit 5fc9cfedfa09199e10b5f9b67dcd286bfeae4f7a:

  Fold send_all() wrapper unix_write() into one function (2010-11-03 12:48:09 -0500)

are available in the git repository at:
  git://repo.or.cz/qemu/kevin.git for-anthony

Blue Swirl (1):
      block: avoid a warning on 64 bit hosts with long as int64_t

Kevin Wolf (9):
      scsi-disk: Implement rerror option
      block: Allow bdrv_flush to return errors
      scsi-disk: Complete failed requests in scsi_disk_emulate_command
      scsi-disk: Implement werror for flushes
      vpc: Implement bdrv_flush
      qcow2: Invalidate cache after failed read
      ide: Handle immediate bdrv_aio_flush failure
      virtio-blk: Handle immediate flush failure properly
      scsi-disk: Fix immediate failure of bdrv_aio_*

 block.c                |   21 ++++++-
 block.h                |    2 +-
 block/blkdebug.c       |    4 +-
 block/blkverify.c      |    8 +-
 block/cow.c            |    4 +-
 block/qcow.c           |    4 +-
 block/qcow2-cluster.c  |    1 +
 block/qcow2-refcount.c |    1 +
 block/qcow2.c          |    4 +-
 block/raw-posix.c      |    4 +-
 block/raw-win32.c      |    9 +++-
 block/raw.c            |    4 +-
 block/vdi.c            |    4 +-
 block/vmdk.c           |    4 +-
 block/vpc.c            |   21 ++++---
 block_int.h            |    2 +-
 blockdev.c             |    6 +-
 hw/ide/core.c          |   12 +++-
 hw/scsi-disk.c         |  147 +++++++++++++++++++++++++++++++----------------
 hw/virtio-blk.c        |    2 +-
 20 files changed, 172 insertions(+), 92 deletions(-)

[Qemu-devel] [PATCH 01/10] scsi-disk: Implement rerror option

[Kevin Wolf]

This implements the rerror option for SCSI disks.

It also includes minor changes to the write path where the same code is used
that was criticized in the review for the changes to the read path required for
rerror support.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 blockdev.c     |    6 ++--
 hw/scsi-disk.c |  100 +++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 69 insertions(+), 37 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ff7602b..6cb179a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -314,7 +314,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     on_write_error = BLOCK_ERR_STOP_ENOSPC;
     if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
         if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
-            fprintf(stderr, "werror is no supported by this format\n");
+            fprintf(stderr, "werror is not supported by this format\n");
             return NULL;
         }
 
@@ -326,8 +326,8 @@ 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) {
-            fprintf(stderr, "rerror is no supported by this format\n");
+        if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
+            fprintf(stderr, "rerror is not supported by this format\n");
             return NULL;
         }
 
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 9628b39..43a5b59 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,29 +178,54 @@ 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;
+    }
+
+    /* No data transfer may already be in progress */
+    assert(r->req.aiocb == NULL);
+
+    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);
+        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
     }
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
-        r->status |= SCSI_REQ_STATUS_RETRY;
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
+
+        type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
+        r->status |= SCSI_REQ_STATUS_RETRY | type;
+
+        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         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);
+        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
 
     return 1;
@@ -214,8 +240,9 @@ static void scsi_write_complete(void * opaque, int ret)
     r->req.aiocb = NULL;
 
     if (ret) {
-        if (scsi_handle_write_error(r, -ret))
+        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_WRITE)) {
             return;
+        }
     }
 
     n = r->iov.iov_len / 512;
@@ -268,8 +295,8 @@ static int scsi_write_data(SCSIDevice *d, uint32_t tag)
         return 1;
     }
 
-    if (r->req.aiocb)
-        BADF("Data transfer already in progress\n");
+    /* No data transfer may already be in progress */
+    assert(r->req.aiocb == NULL);
 
     scsi_write_request(r);
 
@@ -288,8 +315,18 @@ static void scsi_dma_restart_bh(void *opaque)
     QTAILQ_FOREACH(req, &s->qdev.requests, next) {
         r = DO_UPCAST(SCSIDiskReq, req, req);
         if (r->status & SCSI_REQ_STATUS_RETRY) {
-            r->status &= ~SCSI_REQ_STATUS_RETRY;
-            scsi_write_request(r); 
+            int status = r->status;
+            r->status &=
+                ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
+
+            switch (status & SCSI_REQ_STATUS_RETRY_TYPE_MASK) {
+            case SCSI_REQ_STATUS_RETRY_READ:
+                scsi_read_request(r);
+                break;
+            case SCSI_REQ_STATUS_RETRY_WRITE:
+                scsi_write_request(r);
+                break;
+            }
         }
     }
 }
@@ -1152,11 +1189,6 @@ static int scsi_disk_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (bdrv_get_on_error(s->bs, 1) != BLOCK_ERR_REPORT) {
-        error_report("Device doesn't support drive option rerror");
-        return -1;
-    }
-
     if (!s->serial) {
         /* try to fall back to value set with legacy -drive serial=... */
         dinfo = drive_get_by_blockdev(s->bs);
-- 
1.7.2.3

[Qemu-devel] [PATCH 02/10] block: Allow bdrv_flush to return errors

[Kevin Wolf]

This changes bdrv_flush to return 0 on success and -errno in case of failure.
It's a requirement for implementing proper error handle in users of bdrv_flush.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block.c           |   21 +++++++++++++++++----
 block.h           |    2 +-
 block/blkdebug.c  |    4 ++--
 block/blkverify.c |    4 ++--
 block/cow.c       |    4 ++--
 block/qcow.c      |    4 ++--
 block/qcow2.c     |    4 ++--
 block/raw-posix.c |    4 ++--
 block/raw-win32.c |    9 ++++++++-
 block/raw.c       |    4 ++--
 block/vdi.c       |    4 ++--
 block/vmdk.c      |    4 ++--
 block_int.h       |    2 +-
 13 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/block.c b/block.c
index 985d0b7..6b505fb 100644
--- a/block.c
+++ b/block.c
@@ -1453,14 +1453,27 @@ const char *bdrv_get_device_name(BlockDriverState *bs)
     return bs->device_name;
 }
 
-void bdrv_flush(BlockDriverState *bs)
+int bdrv_flush(BlockDriverState *bs)
 {
     if (bs->open_flags & BDRV_O_NO_FLUSH) {
-        return;
+        return 0;
+    }
+
+    if (bs->drv && bs->drv->bdrv_flush) {
+        return bs->drv->bdrv_flush(bs);
     }
 
-    if (bs->drv && bs->drv->bdrv_flush)
-        bs->drv->bdrv_flush(bs);
+    /*
+     * Some block drivers always operate in either writethrough or unsafe mode
+     * and don't support bdrv_flush therefore. Usually qemu doesn't know how
+     * the server works (because the behaviour is hardcoded or depends on
+     * server-side configuration), so we can't ensure that everything is safe
+     * on disk. Returning an error doesn't work because that would break guests
+     * even if the server operates in writethrough mode.
+     *
+     * Let's hope the user knows what he's doing.
+     */
+    return 0;
 }
 
 void bdrv_flush_all(void)
diff --git a/block.h b/block.h
index a4facf2..78ecfac 100644
--- a/block.h
+++ b/block.h
@@ -142,7 +142,7 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque);
 
 /* Ensure contents are flushed to disk.  */
-void bdrv_flush(BlockDriverState *bs);
+int bdrv_flush(BlockDriverState *bs);
 void bdrv_flush_all(void);
 void bdrv_close_all(void);
 
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 4d6ff0a..cd9eb80 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -397,9 +397,9 @@ static void blkdebug_close(BlockDriverState *bs)
     }
 }
 
-static void blkdebug_flush(BlockDriverState *bs)
+static int blkdebug_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *blkdebug_aio_flush(BlockDriverState *bs,
diff --git a/block/blkverify.c b/block/blkverify.c
index b2a12fe..0a8d691 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,12 +116,12 @@ static void blkverify_close(BlockDriverState *bs)
     s->test_file = NULL;
 }
 
-static void blkverify_flush(BlockDriverState *bs)
+static int blkverify_flush(BlockDriverState *bs)
 {
     BDRVBlkverifyState *s = bs->opaque;
 
     /* Only flush test file, the raw file is not important */
-    bdrv_flush(s->test_file);
+    return bdrv_flush(s->test_file);
 }
 
 static int64_t blkverify_getlength(BlockDriverState *bs)
diff --git a/block/cow.c b/block/cow.c
index eedcc48..4cf543c 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -282,9 +282,9 @@ exit:
     return ret;
 }
 
-static void cow_flush(BlockDriverState *bs)
+static int cow_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static QEMUOptionParameter cow_create_options[] = {
diff --git a/block/qcow.c b/block/qcow.c
index 816103d..9cd547d 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -910,9 +910,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
+static int qcow_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *qcow_aio_flush(BlockDriverState *bs,
diff --git a/block/qcow2.c b/block/qcow2.c
index b816d87..537c479 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1148,9 +1148,9 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
+static int qcow_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *qcow_aio_flush(BlockDriverState *bs,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index d0393e0..d0960b8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -734,10 +734,10 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
     return result;
 }
 
-static void raw_flush(BlockDriverState *bs)
+static int raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    qemu_fdatasync(s->fd);
+    return qemu_fdatasync(s->fd);
 }
 
 
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 503ed39..7f32778 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -150,7 +150,14 @@ static int raw_write(BlockDriverState *bs, int64_t sector_num,
 static void raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    FlushFileBuffers(s->hfile);
+    int ret;
+
+    ret = FlushFileBuffers(s->hfile);
+    if (ret != 0) {
+        return -EIO;
+    }
+
+    return 0;
 }
 
 static void raw_close(BlockDriverState *bs)
diff --git a/block/raw.c b/block/raw.c
index 9108779..1980deb 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -39,9 +39,9 @@ static void raw_close(BlockDriverState *bs)
 {
 }
 
-static void raw_flush(BlockDriverState *bs)
+static int raw_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 static BlockDriverAIOCB *raw_aio_flush(BlockDriverState *bs,
diff --git a/block/vdi.c b/block/vdi.c
index f72633c..3b51e53 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -900,10 +900,10 @@ static void vdi_close(BlockDriverState *bs)
 {
 }
 
-static void vdi_flush(BlockDriverState *bs)
+static int vdi_flush(BlockDriverState *bs)
 {
     logout("\n");
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 
diff --git a/block/vmdk.c b/block/vmdk.c
index 2d4ba42..872aeba 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -823,9 +823,9 @@ static void vmdk_close(BlockDriverState *bs)
     qemu_free(s->l2_cache);
 }
 
-static void vmdk_flush(BlockDriverState *bs)
+static int vmdk_flush(BlockDriverState *bs)
 {
-    bdrv_flush(bs->file);
+    return bdrv_flush(bs->file);
 }
 
 
diff --git a/block_int.h b/block_int.h
index 87e60b8..3c3adb5 100644
--- a/block_int.h
+++ b/block_int.h
@@ -59,7 +59,7 @@ struct BlockDriver {
                       const uint8_t *buf, int nb_sectors);
     void (*bdrv_close)(BlockDriverState *bs);
     int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
-    void (*bdrv_flush)(BlockDriverState *bs);
+    int (*bdrv_flush)(BlockDriverState *bs);
     int (*bdrv_is_allocated)(BlockDriverState *bs, int64_t sector_num,
                              int nb_sectors, int *pnum);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
-- 
1.7.2.3

[Qemu-devel] [PATCH 03/10] scsi-disk: Complete failed requests in scsi_disk_emulate_command

[Kevin Wolf]

This pulls the request completion for error cases from the caller to
scsi_disk_emulate_command. This should not change semantics, but allows to
reuse scsi_handle_write_error() for flushes in the next patch.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/scsi-disk.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 43a5b59..96acfe3 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -784,8 +784,9 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf)
     return toclen;
 }
 
-static int scsi_disk_emulate_command(SCSIRequest *req, uint8_t *outbuf)
+static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
 {
+    SCSIRequest *req = &r->req;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint64_t nb_sectors;
     int buflen = 0;
@@ -943,12 +944,12 @@ static int scsi_disk_emulate_command(SCSIRequest *req, uint8_t *outbuf)
     return buflen;
 
 not_ready:
-    scsi_req_set_status(req, CHECK_CONDITION, NOT_READY);
-    return 0;
+    scsi_command_complete(r, CHECK_CONDITION, NOT_READY);
+    return -1;
 
 illegal_request:
-    scsi_req_set_status(req, CHECK_CONDITION, ILLEGAL_REQUEST);
-    return 0;
+    scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+    return -1;
 }
 
 /* Execute a scsi command.  Returns the length of the data expected by the
@@ -1056,14 +1057,12 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     case REPORT_LUNS:
     case VERIFY:
     case REZERO_UNIT:
-        rc = scsi_disk_emulate_command(&r->req, outbuf);
-        if (rc > 0) {
-            r->iov.iov_len = rc;
-        } else {
-            scsi_req_complete(&r->req);
-            scsi_remove_request(r);
+        rc = scsi_disk_emulate_command(r, outbuf);
+        if (rc < 0) {
             return 0;
         }
+
+        r->iov.iov_len = rc;
         break;
     case READ_6:
     case READ_10:
-- 
1.7.2.3

[Qemu-devel] [PATCH 04/10] scsi-disk: Implement werror for flushes

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/scsi-disk.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 96acfe3..6815239 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -45,6 +45,7 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
 #define SCSI_REQ_STATUS_RETRY_READ      0x00
 #define SCSI_REQ_STATUS_RETRY_WRITE     0x02
+#define SCSI_REQ_STATUS_RETRY_FLUSH     0x04
 
 typedef struct SCSIDiskState SCSIDiskState;
 
@@ -74,6 +75,7 @@ struct SCSIDiskState
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
+static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf);
 
 static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
         uint32_t lun)
@@ -316,6 +318,8 @@ static void scsi_dma_restart_bh(void *opaque)
         r = DO_UPCAST(SCSIDiskReq, req, req);
         if (r->status & SCSI_REQ_STATUS_RETRY) {
             int status = r->status;
+            int ret;
+
             r->status &=
                 ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
 
@@ -326,6 +330,11 @@ static void scsi_dma_restart_bh(void *opaque)
             case SCSI_REQ_STATUS_RETRY_WRITE:
                 scsi_write_request(r);
                 break;
+            case SCSI_REQ_STATUS_RETRY_FLUSH:
+                ret = scsi_disk_emulate_command(r, r->iov.iov_base);
+                if (ret == 0) {
+                    scsi_command_complete(r, GOOD, NO_SENSE);
+                }
             }
         }
     }
@@ -790,6 +799,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint64_t nb_sectors;
     int buflen = 0;
+    int ret;
 
     switch (req->cmd.buf[0]) {
     case TEST_UNIT_READY:
@@ -880,7 +890,12 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
         buflen = 8;
 	break;
     case SYNCHRONIZE_CACHE:
-        bdrv_flush(s->bs);
+        ret = bdrv_flush(s->bs);
+        if (ret < 0) {
+            if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_FLUSH)) {
+                return -1;
+            }
+        }
         break;
     case GET_CONFIGURATION:
         memset(outbuf, 0, 8);
-- 
1.7.2.3

[Qemu-devel] [PATCH 05/10] vpc: Implement bdrv_flush

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vpc.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index e50509e..416f489 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -439,6 +439,10 @@ static int vpc_write(BlockDriverState *bs, int64_t sector_num,
     return 0;
 }
 
+static int vpc_flush(BlockDriverState *bs)
+{
+    return bdrv_flush(bs->file);
+}
 
 /*
  * Calculates the number of cylinders, heads and sectors per cylinder
@@ -618,14 +622,15 @@ static QEMUOptionParameter vpc_create_options[] = {
 };
 
 static BlockDriver bdrv_vpc = {
-    .format_name	= "vpc",
-    .instance_size	= sizeof(BDRVVPCState),
-    .bdrv_probe		= vpc_probe,
-    .bdrv_open		= vpc_open,
-    .bdrv_read		= vpc_read,
-    .bdrv_write		= vpc_write,
-    .bdrv_close		= vpc_close,
-    .bdrv_create	= vpc_create,
+    .format_name    = "vpc",
+    .instance_size  = sizeof(BDRVVPCState),
+    .bdrv_probe     = vpc_probe,
+    .bdrv_open      = vpc_open,
+    .bdrv_read      = vpc_read,
+    .bdrv_write     = vpc_write,
+    .bdrv_flush     = vpc_flush,
+    .bdrv_close     = vpc_close,
+    .bdrv_create    = vpc_create,
 
     .create_options = vpc_create_options,
 };
-- 
1.7.2.3

[Qemu-devel] [PATCH 06/10] qcow2: Invalidate cache after failed read

[Kevin Wolf]

The cache content may be destroyed after a failed read, better not use it any
more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/qcow2-cluster.c  |    1 +
 block/qcow2-refcount.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4f7dc59..b040208 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -188,6 +188,7 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
     ret = bdrv_pread(bs->file, l2_offset, *l2_table,
         s->l2_size * sizeof(uint64_t));
     if (ret < 0) {
+        qcow2_l2_cache_reset(bs);
         return ret;
     }
 
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 0efb676..a10453c 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -103,6 +103,7 @@ static int load_refcount_block(BlockDriverState *bs,
     ret = bdrv_pread(bs->file, refcount_block_offset, s->refcount_block_cache,
                      s->cluster_size);
     if (ret < 0) {
+        s->refcount_block_cache_offset = 0;
         return ret;
     }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 07/10] block: avoid a warning on 64 bit hosts with long as int64_t

[Kevin Wolf]

From: Blue Swirl <blauwirbel@gmail.com>

When building on a 64 bit host which uses 'long' for int64_t,
GCC emits a warning:
  CC    block/blkverify.o
/src/qemu/block/blkverify.c: In function `blkverify_verify_readv':
/src/qemu/block/blkverify.c:304: warning: long long int format, long
unsigned int arg (arg 3)

Rework a77cffe7e916f4dd28f2048982ea2e0d98143b11 to avoid the warning.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkverify.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blkverify.c b/block/blkverify.c
index 0a8d691..c7522b4 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -300,8 +300,8 @@ static void blkverify_verify_readv(BlkverifyAIOCB *acb)
 {
     ssize_t offset = blkverify_iovec_compare(acb->qiov, &acb->raw_qiov);
     if (offset != -1) {
-        blkverify_err(acb, "contents mismatch in sector %lld",
-                      acb->sector_num + (offset / BDRV_SECTOR_SIZE));
+        blkverify_err(acb, "contents mismatch in sector %" PRId64,
+                      acb->sector_num + (int64_t)(offset / BDRV_SECTOR_SIZE));
     }
 }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 08/10] ide: Handle immediate bdrv_aio_flush failure

[Kevin Wolf]

If bdrv_aio_flush returns NULL, this should be treated as an error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index bc3e916..484e0ca 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -811,10 +811,16 @@ static void ide_flush_cb(void *opaque, int ret)
 
 static void ide_flush_cache(IDEState *s)
 {
-    if (s->bs) {
-        bdrv_aio_flush(s->bs, ide_flush_cb, s);
-    } else {
+    BlockDriverAIOCB *acb;
+
+    if (s->bs == NULL) {
         ide_flush_cb(s, 0);
+        return;
+    }
+
+    acb = bdrv_aio_flush(s->bs, ide_flush_cb, s);
+    if (acb == NULL) {
+        ide_flush_cb(s, -EIO);
     }
 }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 09/10] virtio-blk: Handle immediate flush failure properly

[Kevin Wolf]

Fix virtio-blk to use the usual completion path that involves werror handling
instead of directly completing the request in cases where bdrv_aio_flush
returns NULL.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/virtio-blk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index dbe2070..49528a9 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -273,7 +273,7 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 
     acb = bdrv_aio_flush(req->dev->bs, virtio_blk_flush_complete, req);
     if (!acb) {
-        virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
+        virtio_blk_flush_complete(req, -EIO);
     }
 }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 10/10] scsi-disk: Fix immediate failure of bdrv_aio_*

[Kevin Wolf]

Fix scsi-disk to use the usual completion paths that involve rerror/werror
handling instead of directly completing the requests in cases where
bdrv_aio_readv/writev returns NULL.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi-disk.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 6815239..dc71957 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -178,8 +178,9 @@ static void scsi_read_request(SCSIDiskReq *r)
     qemu_iovec_init_external(&r->qiov, &r->iov, 1);
     r->req.aiocb = bdrv_aio_readv(s->bs, r->sector, &r->qiov, n,
                               scsi_read_complete, r);
-    if (r->req.aiocb == NULL)
-        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+    if (r->req.aiocb == NULL) {
+        scsi_read_complete(r, -EIO);
+    }
 }
 
 /* Read more data from scsi device into buffer.  */
@@ -273,9 +274,9 @@ static void scsi_write_request(SCSIDiskReq *r)
         qemu_iovec_init_external(&r->qiov, &r->iov, 1);
         r->req.aiocb = bdrv_aio_writev(s->bs, r->sector, &r->qiov, n,
                                    scsi_write_complete, r);
-        if (r->req.aiocb == NULL)
-            scsi_command_complete(r, CHECK_CONDITION,
-                                  HARDWARE_ERROR);
+        if (r->req.aiocb == NULL) {
+            scsi_write_complete(r, -EIO);
+        }
     } else {
         /* Invoke completion routine to fetch data from host.  */
         scsi_write_complete(r, 0);
-- 
1.7.2.3

[Qemu-devel] Re: [PULL 00/10] Block patches

[Anthony Liguori]

On 11/04/2010 08:15 AM, Kevin Wolf wrote:
> The following changes since commit 5fc9cfedfa09199e10b5f9b67dcd286bfeae4f7a:
>
>    Fold send_all() wrapper unix_write() into one function (2010-11-03 12:48:09 -0500)
>
> are available in the git repository at:
>    git://repo.or.cz/qemu/kevin.git for-anthony
>
> Blue Swirl (1):
>        block: avoid a warning on 64 bit hosts with long as int64_t
>
> Kevin Wolf (9):
>        scsi-disk: Implement rerror option
>        block: Allow bdrv_flush to return errors
>        scsi-disk: Complete failed requests in scsi_disk_emulate_command
>        scsi-disk: Implement werror for flushes
>        vpc: Implement bdrv_flush
>        qcow2: Invalidate cache after failed read
>        ide: Handle immediate bdrv_aio_flush failure
>        virtio-blk: Handle immediate flush failure properly
>        scsi-disk: Fix immediate failure of bdrv_aio_*
>    

Pulled.  Thanks.

Regards,

Anthony Liguori

>   block.c                |   21 ++++++-
>   block.h                |    2 +-
>   block/blkdebug.c       |    4 +-
>   block/blkverify.c      |    8 +-
>   block/cow.c            |    4 +-
>   block/qcow.c           |    4 +-
>   block/qcow2-cluster.c  |    1 +
>   block/qcow2-refcount.c |    1 +
>   block/qcow2.c          |    4 +-
>   block/raw-posix.c      |    4 +-
>   block/raw-win32.c      |    9 +++-
>   block/raw.c            |    4 +-
>   block/vdi.c            |    4 +-
>   block/vmdk.c           |    4 +-
>   block/vpc.c            |   21 ++++---
>   block_int.h            |    2 +-
>   blockdev.c             |    6 +-
>   hw/ide/core.c          |   12 +++-
>   hw/scsi-disk.c         |  147 +++++++++++++++++++++++++++++++----------------
>   hw/virtio-blk.c        |    2 +-
>   20 files changed, 172 insertions(+), 92 deletions(-)
>    

[Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Jonathan Nieder]

Hi,

Kevin Wolf wrote:

> This pulls the request completion for error cases from the caller to
> scsi_disk_emulate_command. This should not change semantics, but allows to
> reuse scsi_handle_write_error() for flushes in the next patch.

Today I tried out qemu-system-arm for the first time.  It's faster
than I expected; very neat.  Unfortunately it segfaults.

Reproducible with "master" (077030d11).  Bisects to v0.14.0-rc0~489
(scsi-disk: Complete failed requests in scsi_disk_emulate_command,
2010-10-25).

Ideas?
Jonathan

Backtrace:

| Program received signal SIGSEGV, Segmentation fault.
| 0x00000000005552b5 in lsi_do_command (s=0x13b84d0) at /home/jrn/src/qemu/hw/lsi53c895a.c:762
| 762             dev->info->read_data(dev, s->current->tag);
| (gdb) bt full
| #0  0x00000000005552b5 in lsi_do_command (s=0x13b84d0) at /home/jrn/src/qemu/hw/lsi53c895a.c:762
|         dev = 0x13baf10
|         buf = "\000\000\000\000\000\000\000\000\251\207Q\000\000\000\000"
|         n = 656877154
| #1  lsi_execute_script (s=0x13b84d0) at /home/jrn/src/qemu/hw/lsi53c895a.c:1067
|         insn = 20688656
|         addr = 97263452
|         addr_high = <value optimized out>
|         opcode = <value optimized out>
|         insn_processed = 18
| #2  0x00000000005566b8 in lsi_reg_writeb (s=0x13b84d0, offset=<value optimized out>, val=32 ' ')
|     at /home/jrn/src/qemu/hw/lsi53c895a.c:1656
| No locals.
| #3  0x000000004059fe4e in ?? ()
| No symbol table info available.
| #4  0x0000000000000040 in ?? ()
| No symbol table info available.
| #5  0x0000000000000000 in ?? ()
| No symbol table info available.
| (gdb) p n
| $1 = 656877154
| (gdb) p dev->info
| $2 = (SCSIDeviceInfo *) 0x8df000
| (gdb) p s->current
| $3 = (lsi_request *) 0x0

That's weird because qemu_mallocz should have checked for NULL.

Program counter:

| Dump of assembler code for function lsi_execute_script:
[...]
|    0x0000000000555250 <+2784>:  callq  0x42a970 <qemu_mallocz>
|    0x0000000000555255 <+2789>:  mov    0x334(%rbx),%edx
|    0x000000000055525b <+2795>:  mov    %rax,0x350(%rbx)
|    0x0000000000555262 <+2802>:  mov    %rbp,%rdi
|    0x0000000000555265 <+2805>:  mov    %edx,(%rax)
|    0x0000000000555267 <+2807>:  mov    0x350(%rbx),%rsi
|    0x000000000055526e <+2814>:  lea    0x30(%rsp),%rdx
|    0x0000000000555273 <+2819>:  mov    0x98(%rbp),%rax
|    0x000000000055527a <+2826>:  mov    0x330(%rbx),%ecx
|    0x0000000000555280 <+2832>:  mov    (%rsi),%esi
|    0x0000000000555282 <+2834>:  callq  *0x78(%rax)
|    0x0000000000555285 <+2837>:  cmp    $0x0,%eax
|    0x0000000000555288 <+2840>:  mov    %eax,%r14d
|    0x000000000055528b <+2843>:  jle    0x5555cc <lsi_execute_script+3676>
|    0x0000000000555291 <+2849>:  movzbl 0x38b(%rbx),%eax
|    0x0000000000555298 <+2856>:  mov    0x350(%rbx),%rdx
|    0x000000000055529f <+2863>:  mov    %rbp,%rdi
|    0x00000000005552a2 <+2866>:  and    $0xfffffffffffffff8,%eax
|    0x00000000005552a5 <+2869>:  or     $0x1,%eax
|    0x00000000005552a8 <+2872>:  mov    %al,0x38b(%rbx)
|    0x00000000005552ae <+2878>:  mov    0x98(%rbp),%rax
| => 0x00000000005552b5 <+2885>:  mov    (%rdx),%esi
|    0x00000000005552b7 <+2887>:  callq  *0x80(%rax)
|    0x00000000005552bd <+2893>:  mov    0x338(%rbx),%ebp

Recipe:

| $ ./configure --prefix=$HOME/opt/qemu --disable-werror
| [...]
| $ make -j2 install STRIP=:
| [...]
| $ PATH=$HOME/opt/qemu/bin:$PATH
| $ qemu-img create arm-install.qemu 10G
| Formatting 'arm-install.qemu', fmt=raw size=10737418240
| $ wget http://d-i.debian.org/daily-images/armel/daily/versatile/netboot/initrd.gz
| [...]
| $ wget http://d-i.debian.org/daily-images/armel/daily/versatile/netboot/vmlinuz-2.6.37-2-versatile
| [...]
| $ sha1sum initrd.gz vmlinuz-2.6.37-2-versatile
| 9822cd356e2e66c0ee2d08f2dfc100f074683b81  initrd.gz
| 81aa8f15f6d0fb3fa971d859787f89eec653d1a3  vmlinuz-2.6.37-2-versatile
| $
| $ qemu-system-arm  -M versatilepb -kernel vmlinuz-2.6.37-2-versatile \
|		-initrd initrd.gz -hda arm-install.qemu
| Segmentation fault (core dumped)

The above transcript does not describe the installation process, since
it happened in another window.

1. choice of keymap, mirror, etc are boring
2. It asks for a root password.  Leave it blank.
3. It asks for a new account.  I chose "sudoer".
4. It wants a password.  Give one.
5. Choose a time zone and switch to vt4 for messages.
6. Messages (copied by hand):

| kernel: [  928.454139] SCSI subsystem initialized
| kernel: [  928.767929] PCI: enabling device 0000:00:0c.0 (0100 -> 0103)
| kernel: [  928.840653] sym0: <895a> rev 0x0 at pci 0000:00:0c.0 irq 27
| kernel: [  928.893943] sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking
| kernel: [  928.902942] sym0: SCSI BUS has been reset.
| kernel: [  928.903283] scsi0 : sym-2.2.3
| kernel: [  931.915071] sym0: unknown interrupt(s) ignored, ISTAT=0x5 DSTAT=0x80 SIST=0x0
| kernel: [  931.922015] scsi 0:0:0:0: Direct-Access     QEMU     QEMU HARDDISK    0.14 PQ: 0 ANSI: 5
| kernel: [  931.922765] scsi target0:0:0: tagged command queuing enabled, command queue depth 16.
| kernel: [  931.923171] scsi target0:0:0: Beginning Domain Validation
| kernel: [  931.928165] scsi target0:0:0: Domain Validation skipping write tests

7. Segfault.  The messages stop.

| $ gcc --version
| gcc (Debian 4.6.0-7) 4.6.1 20110507 (prerelease)
| Copyright (C) 2011 Free Software Foundation, Inc.
| This is free software; see the source for copying conditions.  There is NO
| warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
| $ ld --version
| GNU ld (GNU Binutils for Debian) 2.21.51.20110421
| Copyright 2011 Free Software Foundation, Inc.
| This program is free software; you may redistribute it under the terms of
| the GNU General Public License version 3 or (at your option) a later version.
| This program has absolutely no warranty.
| $ /lib/libc.so.6 | head -1
| GNU C Library (Debian EGLIBC 2.13-4) stable release version 2.13, by Roland McGrath et al.
| $ uname -a
| Linux elie 2.6.39-rc5-amd64 #1 SMP Sat Apr 30 05:48:55 UTC 2011 x86_64 GNU/Linux

Re: [Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Kevin Wolf]

Hi Jonathan,

Am 16.05.2011 13:23, schrieb Jonathan Nieder:
> Hi,
> 
> Kevin Wolf wrote:
> 
>> This pulls the request completion for error cases from the caller to
>> scsi_disk_emulate_command. This should not change semantics, but allows to
>> reuse scsi_handle_write_error() for flushes in the next patch.
> 
> Today I tried out qemu-system-arm for the first time.  It's faster
> than I expected; very neat.  Unfortunately it segfaults.
> 
> Reproducible with "master" (077030d11).  Bisects to v0.14.0-rc0~489
> (scsi-disk: Complete failed requests in scsi_disk_emulate_command,
> 2010-10-25).

Your instructions seemed clear enough, so I tried to reproduce your
problem. Now I have an ARM VM with a Debian installation that works just
fine and I have no idea what to use it for. ;-)

I also reviewed the patch that you mentioned and I can't find anything
suspicious there. I'm afraid you'll have to bite the bullet and run it
with some debugging code yourself (if it's really related to that patch,
you'll want to enable DPRINTF in hw/scsi-disk.c as a first step)

Kevin

Re: [Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Jonathan Nieder]

Kevin Wolf wrote:

> I also reviewed the patch that you mentioned and I can't find anything
> suspicious there. I'm afraid you'll have to bite the bullet and run it
> with some debugging code yourself (if it's really related to that patch,
> you'll want to enable DPRINTF in hw/scsi-disk.c as a first step)

I tried reverting a6d96eb7 (scsi: Move sense handling into the driver,
2010-11-24), 78ced65e (scsi-disk: Implement werror for flushes,
2010-10-25), and 8af7a3a (csi-disk: Complete failed requests in
scsi_disk_emulate_command, 2010-10-25), and the segfault is gone.  So
now I also have a nice ARM image to reproduce it more quickly with. :)

Here's what the default DPRINTFs write when it segfaults, for what
it's worth.  I'll try playing with this some more.

scsi-disk: Command: lun=0 tag=0x0 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x0 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10001 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10001 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10003 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10003 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10005 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10005 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10007 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10007 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10009 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10009 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x1000b data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x1000b status=0 sense=0
scsi-disk: Command: lun=0 tag=0x1000d data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x1000d status=0 sense=0
scsi-disk: Command: lun=0 tag=0x1000f data=0xa0 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x00 0x00 0x00
scsi-disk: Read buf_len=16
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x1000f status=0 sense=0
scsi-disk: Command: lun=0 tag=0x200 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x200 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10201 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10201 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10203 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10203 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10205 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10205 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10207 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10207 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10209 data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x10209 status=0 sense=0
scsi-disk: Command: lun=0 tag=0x1020b data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x1020b status=0 sense=0
scsi-disk: Command: lun=0 tag=0x1020d data=0x12 0x00 0x00 0x00 0x24 0x00
scsi-disk: Read buf_len=36
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x1020d status=0 sense=0
scsi-disk: Command: lun=0 tag=0x1020f data=0xa0 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x00 0x00 0x00
scsi-disk: Read buf_len=16
scsi-disk: Read sector_count=0
scsi-disk: Command complete tag=0x1020f status=0 sense=0
scsi-disk: Command: lun=0 tag=0x10011 data=0x00 0x00 0x00 0x00 0x00 0x00
scsi-disk: Command complete tag=0x10011 status=0 sense=0

Re: [Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Jonathan Nieder]

Kevin Wolf wrote:

> Your instructions seemed clear enough, so I tried to reproduce your
> problem. Now I have an ARM VM with a Debian installation that works just
> fine and I have no idea what to use it for. ;-)

So I was puzzled about this for a while, but then I had a flash
of inspiration:

	unset MALLOC_PERTURB_
	reproduction-script;	# no segfault

	MALLOC_PERTURB_=37
	export MALLOC_PERTURB_
	reproduction-script;	# segfaults

Thanks.  Sorry, it's easy to forget.

Re: [Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Kevin Wolf]

Am 16.05.2011 17:43, schrieb Jonathan Nieder:
> Kevin Wolf wrote:
> 
>> Your instructions seemed clear enough, so I tried to reproduce your
>> problem. Now I have an ARM VM with a Debian installation that works just
>> fine and I have no idea what to use it for. ;-)
> 
> So I was puzzled about this for a while, but then I had a flash
> of inspiration:
> 
> 	unset MALLOC_PERTURB_
> 	reproduction-script;	# no segfault
> 
> 	MALLOC_PERTURB_=37
> 	export MALLOC_PERTURB_
> 	reproduction-script;	# segfaults
> 
> Thanks.  Sorry, it's easy to forget.

Thanks. Still doesn't make much sense to me, the patch shouldn't change
anything with respect to a malloc, but I can reproduce a segfault now. I
think I'll have a closer look tomorrow.

Kevin

Re: [Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Paolo Bonzini]

On 05/16/2011 05:58 PM, Kevin Wolf wrote:
> Thanks. Still doesn't make much sense to me, the patch shouldn't change
> anything with respect to a malloc, but I can reproduce a segfault now. I
> think I'll have a closer look tomorrow.

This fixes it on top of my SCSI refactoring series.  Should I send v3
with this one squashed in appropriately?  Or should this be sent later?

Paolo

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 2f0ffda..57cfc87 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -167,11 +167,17 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int len)
 
 int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
 {
+    int32_t rc;
     assert(!req->enqueued);
     scsi_req_ref(req);
     req->enqueued = true;
     QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
-    return req->dev->info->send_command(req, buf);
+
+    /* Make sure the request doesn't disappear under send_command's feet.  */
+    scsi_req_ref(req);
+    rc = req->dev->info->send_command(req, buf);
+    scsi_req_unref(req);
+    return rc;
 }
 
 static void scsi_req_dequeue(SCSIRequest *req)

Re: [Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Jonathan Nieder]

Paolo Bonzini wrote:

> This fixes it on top of my SCSI refactoring series.

Thanks!  Works here, too, for what it's worth.

I squashed the following in when applying the "scsi: introduce
scsi_req_cancel" patch, for easier reading and to get a little closer
to warning-free compilation with gcc 4.6.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 hw/lsi53c895a.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f291283..43de6f8 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -889,7 +889,6 @@ static void lsi_do_msgout(LSIState *s)
     uint8_t msg;
     int len;
     uint32_t current_tag;
-    SCSIDevice *current_dev;
     lsi_request *current_req, *p, *p_next;
     int id;
 
@@ -900,8 +899,6 @@ static void lsi_do_msgout(LSIState *s)
         current_tag = s->select_tag;
         current_req = lsi_find_by_tag(s, current_tag);
     }
-    id = (current_tag >> 8) & 0xf;
-    current_dev = s->bus.devs[id];
 
     DPRINTF("MSG out len=%d\n", s->dbc);
     while (s->dbc) {
-- 
1.7.5.1

Re: [Qemu-devel] [regression] qemu-system-arm: segfault in lsi_do_command

[Kevin Wolf]

Am 16.05.2011 18:26, schrieb Paolo Bonzini:
> On 05/16/2011 05:58 PM, Kevin Wolf wrote:
>> Thanks. Still doesn't make much sense to me, the patch shouldn't change
>> anything with respect to a malloc, but I can reproduce a segfault now. I
>> think I'll have a closer look tomorrow.
> 
> This fixes it on top of my SCSI refactoring series.  Should I send v3
> with this one squashed in appropriately?  Or should this be sent later?

I think I would just include it in your series (and while you're at it,
I think you could include Jonathan's cleanup as well).

Kevin