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

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

[Kevin Wolf]

The following changes since commit 84988cf910a6881f2180fdcec516b60f8f0dc8c4:

  bitops.h: Add functions to extract and deposit bitfields (2012-07-07 09:07:01 +0000)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git for-anthony

MORITA Kazutaka (6):
      sheepdog: fix dprintf format strings
      sheepdog: restart I/O when socket becomes ready in do_co_req()
      sheepdog: use coroutine based socket functions in coroutine context
      sheepdog: make sure we don't free aiocb before sending all requests
      sheepdog: split outstanding list into inflight and pending
      sheepdog: traverse pending_list from the first for each time

Markus Armbruster (4):
      fdc: Drop broken code for user-defined floppy geometry
      fdc: Move floppy geometry guessing back from block.c
      qtest: Tidy up temporary files properly
      block: Factor bdrv_read_unthrottled() out of guess_disk_lchs()

Paolo Bonzini (8):
      blkdebug: remove sync i/o events
      blkdebug: tiny cleanup
      blkdebug: pass getlength to underlying file
      blkdebug: store list of active rules
      blkdebug: optionally tie errors to a specific sector
      raw: hook into blkdebug
      block: copy over job and dirty bitmap fields in bdrv_append
      block: introduce bdrv_swap, implement bdrv_append on top of it

Pavel Hrdina (4):
      fdc: rewrite seek and DSKCHG bit handling
      fdc: fix interrupt handling
      fdc_test: update media_change test
      fdc_test: introduce test_sense_interrupt

Stefan Hajnoczi (3):
      qcow2: fix #ifdef'd qcow2_check_refcounts() callers
      qcow2: preserve free_byte_offset when qcow2_alloc_bytes() fails
      blockdev: warn when copy_on_read=on and readonly=on

 block.c                |  306 +++++++++++++++++++-----------------------------
 block.h                |   23 +---
 block/blkdebug.c       |  107 ++++++++++-------
 block/qcow2-refcount.c |    7 +-
 block/qcow2-snapshot.c |    6 +-
 block/qcow2.c          |    2 +-
 block/qed.c            |    2 +-
 block/raw.c            |    2 +
 block/sheepdog.c       |  130 +++++++++++++--------
 blockdev.c             |    4 +
 hw/fdc.c               |  238 +++++++++++++++++++++++++++----------
 hw/fdc.h               |   10 ++-
 hw/pc.c                |   13 +--
 tests/fdc-test.c       |   50 +++++++--
 tests/libqtest.c       |   29 +++--
 15 files changed, 522 insertions(+), 407 deletions(-)

[Qemu-devel] [PATCH 01/25] qcow2: fix #ifdef'd qcow2_check_refcounts() callers

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

The DEBUG_ALLOC qcow2.h macro enables additional consistency checks
throughout the code.  This makes it easier to spot corruptions that are
introduced during development.  Since consistency check is an expensive
operation the DEBUG_ALLOC macro is used to compile checks out in normal
builds and qcow2_check_refcounts() calls missed the addition of a new
function argument.

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

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4561a2a..4e7c93b 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -405,7 +405,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 #ifdef DEBUG_ALLOC
     {
       BdrvCheckResult result = {0};
-      qcow2_check_refcounts(bs, &result);
+      qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
     return 0;
@@ -522,7 +522,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};
-        qcow2_check_refcounts(bs, &result);
+        qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
     return 0;
@@ -582,7 +582,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};
-        qcow2_check_refcounts(bs, &result);
+        qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
     return 0;
diff --git a/block/qcow2.c b/block/qcow2.c
index 2c1cd0a..5be5ace 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -415,7 +415,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};
-        qcow2_check_refcounts(bs, &result);
+        qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
     return ret;
-- 
1.7.6.5

[Qemu-devel] [PATCH 02/25] qcow2: preserve free_byte_offset when qcow2_alloc_bytes() fails

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

When qcow2_alloc_clusters() error handling code was introduced in commit
5d757b563d59142ca81e1073a8e8396750a0ad1a, the value of free_byte_offset
was clobbered in the error case.  This patch keeps free_byte_offset at 0
so we will try to allocate clusters again next time this function is
called.

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

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 66f3915..5e3f915 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -627,10 +627,11 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
     BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES);
     assert(size > 0 && size <= s->cluster_size);
     if (s->free_byte_offset == 0) {
-        s->free_byte_offset = qcow2_alloc_clusters(bs, s->cluster_size);
-        if (s->free_byte_offset < 0) {
-            return s->free_byte_offset;
+        offset = qcow2_alloc_clusters(bs, s->cluster_size);
+        if (offset < 0) {
+            return offset;
         }
+        s->free_byte_offset = offset;
     }
  redo:
     free_in_cluster = s->cluster_size -
-- 
1.7.6.5

[Qemu-devel] [PATCH 03/25] blockdev: warn when copy_on_read=on and readonly=on

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

If the image is read-only then it's not possible to copy read data into
it.  Therefore copy-on-read is automatically disabled for read-only
images.

Up until now this behavior was silent, add a warning so the user knows
why copy-on-read is not working.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9e0a72a..a85a429 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -609,6 +609,10 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
+    if (ro && copy_on_read) {
+        error_report("warning: disabling copy_on_read on readonly drive");
+    }
+
     ret = bdrv_open(dinfo->bdrv, file, bdrv_flags, drv);
     if (ret < 0) {
         error_report("could not open disk image %s: %s",
-- 
1.7.6.5

[Qemu-devel] [PATCH 04/25] sheepdog: fix dprintf format strings

[Kevin Wolf]

From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

This fixes warnings about dprintf format in debug mode.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 8877f45..afd06aa 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1571,11 +1571,11 @@ static int coroutine_fn sd_co_rw_vector(void *p)
         }
 
         if (create) {
-            dprintf("update ino (%" PRIu32") %" PRIu64 " %" PRIu64
-                    " %" PRIu64 "\n", inode->vdi_id, oid,
+            dprintf("update ino (%" PRIu32 ") %" PRIu64 " %" PRIu64 " %ld\n",
+                    inode->vdi_id, oid,
                     vid_to_data_oid(inode->data_vdi_id[idx], idx), idx);
             oid = vid_to_data_oid(inode->vdi_id, idx);
-            dprintf("new oid %lx\n", oid);
+            dprintf("new oid %" PRIx64 "\n", oid);
         }
 
         aio_req = alloc_aio_req(s, acb, oid, len, offset, flags, old_oid, done);
@@ -1726,7 +1726,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     SheepdogInode *inode;
     unsigned int datalen;
 
-    dprintf("sn_info: name %s id_str %s s: name %s vm_state_size %d "
+    dprintf("sn_info: name %s id_str %s s: name %s vm_state_size %" PRId64 " "
             "is_snapshot %d\n", sn_info->name, sn_info->id_str,
             s->name, sn_info->vm_state_size, s->is_snapshot);
 
-- 
1.7.6.5

[Qemu-devel] [PATCH 05/25] sheepdog: restart I/O when socket becomes ready in do_co_req()

[Kevin Wolf]

From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

Currently, no one reenters the yielded coroutine.  This fixes it.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index afd06aa..0b49c6d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -577,10 +577,21 @@ out:
     return ret;
 }
 
+static void restart_co_req(void *opaque)
+{
+    Coroutine *co = opaque;
+
+    qemu_coroutine_enter(co, NULL);
+}
+
 static coroutine_fn int do_co_req(int sockfd, SheepdogReq *hdr, void *data,
                                   unsigned int *wlen, unsigned int *rlen)
 {
     int ret;
+    Coroutine *co;
+
+    co = qemu_coroutine_self();
+    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, NULL, co);
 
     socket_set_block(sockfd);
     ret = send_co_req(sockfd, hdr, data, wlen);
@@ -588,6 +599,8 @@ static coroutine_fn int do_co_req(int sockfd, SheepdogReq *hdr, void *data,
         goto out;
     }
 
+    qemu_aio_set_fd_handler(sockfd, restart_co_req, NULL, NULL, co);
+
     ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
     if (ret < sizeof(*hdr)) {
         error_report("failed to get a rsp, %s", strerror(errno));
@@ -609,6 +622,7 @@ static coroutine_fn int do_co_req(int sockfd, SheepdogReq *hdr, void *data,
     }
     ret = 0;
 out:
+    qemu_aio_set_fd_handler(sockfd, NULL, NULL, NULL, NULL);
     socket_set_nonblock(sockfd);
     return ret;
 }
-- 
1.7.6.5

[Qemu-devel] [PATCH 06/25] sheepdog: use coroutine based socket functions in coroutine context

[Kevin Wolf]

From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

This removes blocking network I/Os in coroutine context.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0b49c6d..5dc1d7a 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -541,11 +541,18 @@ static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
     return ret;
 }
 
+static coroutine_fn int do_co_req(int sockfd, SheepdogReq *hdr, void *data,
+                                  unsigned int *wlen, unsigned int *rlen);
+
 static int do_req(int sockfd, SheepdogReq *hdr, void *data,
                   unsigned int *wlen, unsigned int *rlen)
 {
     int ret;
 
+    if (qemu_in_coroutine()) {
+        return do_co_req(sockfd, hdr, data, wlen, rlen);
+    }
+
     socket_set_block(sockfd);
     ret = send_req(sockfd, hdr, data, wlen);
     if (ret < 0) {
@@ -1642,7 +1649,6 @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
     int ret;
 
     if (bs->growable && sector_num + nb_sectors > bs->total_sectors) {
-        /* TODO: shouldn't block here */
         ret = sd_truncate(bs, (sector_num + nb_sectors) * SECTOR_SIZE);
         if (ret < 0) {
             return ret;
@@ -1710,7 +1716,7 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
     hdr.opcode = SD_OP_FLUSH_VDI;
     hdr.oid = vid_to_vdi_oid(inode->vdi_id);
 
-    ret = do_co_req(s->flush_fd, (SheepdogReq *)&hdr, NULL, &wlen, &rlen);
+    ret = do_req(s->flush_fd, (SheepdogReq *)&hdr, NULL, &wlen, &rlen);
     if (ret) {
         error_report("failed to send a request to the sheep");
         return ret;
-- 
1.7.6.5

[Qemu-devel] [PATCH 07/25] sheepdog: make sure we don't free aiocb before sending all requests

[Kevin Wolf]

From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

This patch increments the pending counter before sending requests, and
make sures that aiocb is not freed while sending them.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 5dc1d7a..d4e5e3a 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -260,7 +260,6 @@ typedef struct AIOReq {
     uint32_t id;
 
     QLIST_ENTRY(AIOReq) outstanding_aio_siblings;
-    QLIST_ENTRY(AIOReq) aioreq_siblings;
 } AIOReq;
 
 enum AIOCBState {
@@ -283,8 +282,7 @@ struct SheepdogAIOCB {
     void (*aio_done_func)(SheepdogAIOCB *);
 
     int canceled;
-
-    QLIST_HEAD(aioreq_head, AIOReq) aioreq_head;
+    int nr_pending;
 };
 
 typedef struct BDRVSheepdogState {
@@ -388,19 +386,19 @@ static inline AIOReq *alloc_aio_req(BDRVSheepdogState *s, SheepdogAIOCB *acb,
 
     QLIST_INSERT_HEAD(&s->outstanding_aio_head, aio_req,
                       outstanding_aio_siblings);
-    QLIST_INSERT_HEAD(&acb->aioreq_head, aio_req, aioreq_siblings);
 
+    acb->nr_pending++;
     return aio_req;
 }
 
-static inline int free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
+static inline void free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
 {
     SheepdogAIOCB *acb = aio_req->aiocb;
+
     QLIST_REMOVE(aio_req, outstanding_aio_siblings);
-    QLIST_REMOVE(aio_req, aioreq_siblings);
     g_free(aio_req);
 
-    return !QLIST_EMPTY(&acb->aioreq_head);
+    acb->nr_pending--;
 }
 
 static void coroutine_fn sd_finish_aiocb(SheepdogAIOCB *acb)
@@ -446,7 +444,7 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
     acb->canceled = 0;
     acb->coroutine = qemu_coroutine_self();
     acb->ret = 0;
-    QLIST_INIT(&acb->aioreq_head);
+    acb->nr_pending = 0;
     return acb;
 }
 
@@ -663,7 +661,7 @@ static void coroutine_fn send_pending_req(BDRVSheepdogState *s, uint64_t oid, ui
         if (ret < 0) {
             error_report("add_aio_request is failed");
             free_aio_req(s, aio_req);
-            if (QLIST_EMPTY(&acb->aioreq_head)) {
+            if (!acb->nr_pending) {
                 sd_finish_aiocb(acb);
             }
         }
@@ -684,7 +682,6 @@ static void coroutine_fn aio_read_response(void *opaque)
     int ret;
     AIOReq *aio_req = NULL;
     SheepdogAIOCB *acb;
-    int rest;
     unsigned long idx;
 
     if (QLIST_EMPTY(&s->outstanding_aio_head)) {
@@ -755,8 +752,8 @@ static void coroutine_fn aio_read_response(void *opaque)
         error_report("%s", sd_strerror(rsp.result));
     }
 
-    rest = free_aio_req(s, aio_req);
-    if (!rest) {
+    free_aio_req(s, aio_req);
+    if (!acb->nr_pending) {
         /*
          * We've finished all requests which belong to the AIOCB, so
          * we can switch back to sd_co_readv/writev now.
@@ -1568,6 +1565,12 @@ static int coroutine_fn sd_co_rw_vector(void *p)
         }
     }
 
+    /*
+     * Make sure we don't free the aiocb before we are done with all requests.
+     * This additional reference is dropped at the end of this function.
+     */
+    acb->nr_pending++;
+
     while (done != total) {
         uint8_t flags = 0;
         uint64_t old_oid = 0;
@@ -1636,7 +1639,7 @@ static int coroutine_fn sd_co_rw_vector(void *p)
         done += len;
     }
 out:
-    if (QLIST_EMPTY(&acb->aioreq_head)) {
+    if (!--acb->nr_pending) {
         return acb->ret;
     }
     return 1;
-- 
1.7.6.5

[Qemu-devel] [PATCH 08/25] sheepdog: split outstanding list into inflight and pending

[Kevin Wolf]

From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

outstanding_list_head is used for both pending and inflight requests.
This patch splits it and improves readability.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c |   49 ++++++++++++++++++++++++-------------------------
 1 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index d4e5e3a..f6cd517 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -259,7 +259,7 @@ typedef struct AIOReq {
     uint8_t flags;
     uint32_t id;
 
-    QLIST_ENTRY(AIOReq) outstanding_aio_siblings;
+    QLIST_ENTRY(AIOReq) aio_siblings;
 } AIOReq;
 
 enum AIOCBState {
@@ -305,7 +305,8 @@ typedef struct BDRVSheepdogState {
     Coroutine *co_recv;
 
     uint32_t aioreq_seq_num;
-    QLIST_HEAD(outstanding_aio_head, AIOReq) outstanding_aio_head;
+    QLIST_HEAD(inflight_aio_head, AIOReq) inflight_aio_head;
+    QLIST_HEAD(pending_aio_head, AIOReq) pending_aio_head;
 } BDRVSheepdogState;
 
 static const char * sd_strerror(int err)
@@ -356,7 +357,7 @@ static const char * sd_strerror(int err)
  * Sheepdog I/O handling:
  *
  * 1. In sd_co_rw_vector, we send the I/O requests to the server and
- *    link the requests to the outstanding_list in the
+ *    link the requests to the inflight_list in the
  *    BDRVSheepdogState.  The function exits without waiting for
  *    receiving the response.
  *
@@ -384,9 +385,6 @@ static inline AIOReq *alloc_aio_req(BDRVSheepdogState *s, SheepdogAIOCB *acb,
     aio_req->flags = flags;
     aio_req->id = s->aioreq_seq_num++;
 
-    QLIST_INSERT_HEAD(&s->outstanding_aio_head, aio_req,
-                      outstanding_aio_siblings);
-
     acb->nr_pending++;
     return aio_req;
 }
@@ -395,7 +393,7 @@ static inline void free_aio_req(BDRVSheepdogState *s, AIOReq *aio_req)
 {
     SheepdogAIOCB *acb = aio_req->aiocb;
 
-    QLIST_REMOVE(aio_req, outstanding_aio_siblings);
+    QLIST_REMOVE(aio_req, aio_siblings);
     g_free(aio_req);
 
     acb->nr_pending--;
@@ -640,22 +638,21 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
  * This function searchs pending requests to the object `oid', and
  * sends them.
  */
-static void coroutine_fn send_pending_req(BDRVSheepdogState *s, uint64_t oid, uint32_t id)
+static void coroutine_fn send_pending_req(BDRVSheepdogState *s, uint64_t oid)
 {
     AIOReq *aio_req, *next;
     SheepdogAIOCB *acb;
     int ret;
 
-    QLIST_FOREACH_SAFE(aio_req, &s->outstanding_aio_head,
-                       outstanding_aio_siblings, next) {
-        if (id == aio_req->id) {
-            continue;
-        }
+    QLIST_FOREACH_SAFE(aio_req, &s->pending_aio_head, aio_siblings, next) {
         if (aio_req->oid != oid) {
             continue;
         }
 
         acb = aio_req->aiocb;
+        /* move aio_req from pending list to inflight one */
+        QLIST_REMOVE(aio_req, aio_siblings);
+        QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
         ret = add_aio_request(s, aio_req, acb->qiov->iov,
                               acb->qiov->niov, 0, acb->aiocb_type);
         if (ret < 0) {
@@ -684,7 +681,7 @@ static void coroutine_fn aio_read_response(void *opaque)
     SheepdogAIOCB *acb;
     unsigned long idx;
 
-    if (QLIST_EMPTY(&s->outstanding_aio_head)) {
+    if (QLIST_EMPTY(&s->inflight_aio_head)) {
         goto out;
     }
 
@@ -695,8 +692,8 @@ static void coroutine_fn aio_read_response(void *opaque)
         goto out;
     }
 
-    /* find the right aio_req from the outstanding_aio list */
-    QLIST_FOREACH(aio_req, &s->outstanding_aio_head, outstanding_aio_siblings) {
+    /* find the right aio_req from the inflight aio list */
+    QLIST_FOREACH(aio_req, &s->inflight_aio_head, aio_siblings) {
         if (aio_req->id == rsp.id) {
             break;
         }
@@ -734,7 +731,7 @@ static void coroutine_fn aio_read_response(void *opaque)
              * create requests are not allowed, so we search the
              * pending requests here.
              */
-            send_pending_req(s, vid_to_data_oid(s->inode.vdi_id, idx), rsp.id);
+            send_pending_req(s, vid_to_data_oid(s->inode.vdi_id, idx));
         }
         break;
     case AIOCB_READ_UDATA:
@@ -786,7 +783,8 @@ static int aio_flush_request(void *opaque)
 {
     BDRVSheepdogState *s = opaque;
 
-    return !QLIST_EMPTY(&s->outstanding_aio_head);
+    return !QLIST_EMPTY(&s->inflight_aio_head) ||
+        !QLIST_EMPTY(&s->pending_aio_head);
 }
 
 static int set_nodelay(int fd)
@@ -1103,7 +1101,8 @@ static int sd_open(BlockDriverState *bs, const char *filename, int flags)
 
     strstart(filename, "sheepdog:", (const char **)&filename);
 
-    QLIST_INIT(&s->outstanding_aio_head);
+    QLIST_INIT(&s->inflight_aio_head);
+    QLIST_INIT(&s->pending_aio_head);
     s->fd = -1;
 
     memset(vdi, 0, sizeof(vdi));
@@ -1465,6 +1464,7 @@ static void coroutine_fn sd_write_done(SheepdogAIOCB *acb)
         iov.iov_len = sizeof(s->inode);
         aio_req = alloc_aio_req(s, acb, vid_to_vdi_oid(s->inode.vdi_id),
                                 data_len, offset, 0, 0, offset);
+        QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
         ret = add_aio_request(s, aio_req, &iov, 1, 0, AIOCB_WRITE_UDATA);
         if (ret) {
             free_aio_req(s, aio_req);
@@ -1533,7 +1533,7 @@ out:
  * Send I/O requests to the server.
  *
  * This function sends requests to the server, links the requests to
- * the outstanding_list in BDRVSheepdogState, and exits without
+ * the inflight_list in BDRVSheepdogState, and exits without
  * waiting the response.  The responses are received in the
  * `aio_read_response' function which is called from the main loop as
  * a fd handler.
@@ -1606,11 +1606,7 @@ static int coroutine_fn sd_co_rw_vector(void *p)
 
         if (create) {
             AIOReq *areq;
-            QLIST_FOREACH(areq, &s->outstanding_aio_head,
-                          outstanding_aio_siblings) {
-                if (areq == aio_req) {
-                    continue;
-                }
+            QLIST_FOREACH(areq, &s->inflight_aio_head, aio_siblings) {
                 if (areq->oid == oid) {
                     /*
                      * Sheepdog cannot handle simultaneous create
@@ -1620,11 +1616,14 @@ static int coroutine_fn sd_co_rw_vector(void *p)
                      */
                     aio_req->flags = 0;
                     aio_req->base_oid = 0;
+                    QLIST_INSERT_HEAD(&s->pending_aio_head, aio_req,
+                                      aio_siblings);
                     goto done;
                 }
             }
         }
 
+        QLIST_INSERT_HEAD(&s->inflight_aio_head, aio_req, aio_siblings);
         ret = add_aio_request(s, aio_req, acb->qiov->iov, acb->qiov->niov,
                               create, acb->aiocb_type);
         if (ret < 0) {
-- 
1.7.6.5

[Qemu-devel] [PATCH 09/25] sheepdog: traverse pending_list from the first for each time

[Kevin Wolf]

From: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

The pending list can be modified in other coroutine context
sd_co_rw_vector, so we need to traverse the list from the first again
after we send the pending request.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index f6cd517..6e73efb 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -634,21 +634,31 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
                            struct iovec *iov, int niov, int create,
                            enum AIOCBState aiocb_type);
 
+
+static AIOReq *find_pending_req(BDRVSheepdogState *s, uint64_t oid)
+{
+    AIOReq *aio_req;
+
+    QLIST_FOREACH(aio_req, &s->pending_aio_head, aio_siblings) {
+        if (aio_req->oid == oid) {
+            return aio_req;
+        }
+    }
+
+    return NULL;
+}
+
 /*
  * This function searchs pending requests to the object `oid', and
  * sends them.
  */
 static void coroutine_fn send_pending_req(BDRVSheepdogState *s, uint64_t oid)
 {
-    AIOReq *aio_req, *next;
+    AIOReq *aio_req;
     SheepdogAIOCB *acb;
     int ret;
 
-    QLIST_FOREACH_SAFE(aio_req, &s->pending_aio_head, aio_siblings, next) {
-        if (aio_req->oid != oid) {
-            continue;
-        }
-
+    while ((aio_req = find_pending_req(s, oid)) != NULL) {
         acb = aio_req->aiocb;
         /* move aio_req from pending list to inflight one */
         QLIST_REMOVE(aio_req, aio_siblings);
-- 
1.7.6.5

[Qemu-devel] [PATCH 10/25] blkdebug: remove sync i/o events

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

These are unused, except (by mistake more or less) in QED.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.h          |    2 --
 block/blkdebug.c |    2 --
 block/qed.c      |    2 +-
 3 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/block.h b/block.h
index d135652..42e30d6 100644
--- a/block.h
+++ b/block.h
@@ -395,9 +395,7 @@ typedef enum {
     BLKDBG_L2_ALLOC_COW_READ,
     BLKDBG_L2_ALLOC_WRITE,
 
-    BLKDBG_READ,
     BLKDBG_READ_AIO,
-    BLKDBG_READ_BACKING,
     BLKDBG_READ_BACKING_AIO,
     BLKDBG_READ_COMPRESSED,
 
diff --git a/block/blkdebug.c b/block/blkdebug.c
index e56e37d..1eff940 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -147,9 +147,7 @@ static const char *event_names[BLKDBG_EVENT_MAX] = {
     [BLKDBG_L2_ALLOC_COW_READ]              = "l2_alloc.cow_read",
     [BLKDBG_L2_ALLOC_WRITE]                 = "l2_alloc.write",
 
-    [BLKDBG_READ]                           = "read",
     [BLKDBG_READ_AIO]                       = "read_aio",
-    [BLKDBG_READ_BACKING]                   = "read_backing",
     [BLKDBG_READ_BACKING_AIO]               = "read_backing_aio",
     [BLKDBG_READ_COMPRESSED]                = "read_compressed",
 
diff --git a/block/qed.c b/block/qed.c
index ab59724..dd2832a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -748,7 +748,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     /* If the read straddles the end of the backing file, shorten it */
     size = MIN((uint64_t)backing_length - pos, qiov->size);
 
-    BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING);
+    BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO);
     bdrv_aio_readv(s->bs->backing_hd, pos / BDRV_SECTOR_SIZE,
                    qiov, size / BDRV_SECTOR_SIZE, cb, opaque);
 }
-- 
1.7.6.5

[Qemu-devel] [PATCH 11/25] blkdebug: tiny cleanup

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 1eff940..1f79ef2 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -361,9 +361,7 @@ static BlockDriverAIOCB *blkdebug_aio_readv(BlockDriverState *bs,
         return inject_error(bs, cb, opaque);
     }
 
-    BlockDriverAIOCB *acb =
-        bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
-    return acb;
+    return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
 }
 
 static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
@@ -376,9 +374,7 @@ static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
         return inject_error(bs, cb, opaque);
     }
 
-    BlockDriverAIOCB *acb =
-        bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
-    return acb;
+    return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
 }
 
 static void blkdebug_close(BlockDriverState *bs)
-- 
1.7.6.5

[Qemu-devel] [PATCH 12/25] blkdebug: pass getlength to underlying file

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

This is required when using blkdebug with raw format.  Unlike qcow2/QED,
raw asks blkdebug for the length of the file, it doesn't get it from
a header.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 1f79ef2..b084a23 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -429,6 +429,11 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event)
     }
 }
 
+static int64_t blkdebug_getlength(BlockDriverState *bs)
+{
+    return bdrv_getlength(bs->file);
+}
+
 static BlockDriver bdrv_blkdebug = {
     .format_name        = "blkdebug",
     .protocol_name      = "blkdebug",
@@ -437,6 +442,7 @@ static BlockDriver bdrv_blkdebug = {
 
     .bdrv_file_open     = blkdebug_open,
     .bdrv_close         = blkdebug_close,
+    .bdrv_getlength     = blkdebug_getlength,
 
     .bdrv_aio_readv     = blkdebug_aio_readv,
     .bdrv_aio_writev    = blkdebug_aio_writev,
-- 
1.7.6.5

[Qemu-devel] [PATCH 13/25] blkdebug: store list of active rules

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

This prepares for the next patch, where some active rules may actually
not trigger depending on input to readv/writev.  Store the active rules
in a SIMPLEQ (so that it can be emptied easily with QSIMPLEQ_INIT), and
fetch the errno/once/immediately arguments from there.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c |   69 ++++++++++++++++++++++++-----------------------------
 1 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index b084a23..d12ebbf 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -26,24 +26,10 @@
 #include "block_int.h"
 #include "module.h"
 
-typedef struct BlkdebugVars {
-    int state;
-
-    /* If inject_errno != 0, an error is injected for requests */
-    int inject_errno;
-
-    /* Decides if all future requests fail (false) or only the next one and
-     * after the next request inject_errno is reset to 0 (true) */
-    bool inject_once;
-
-    /* Decides if aio_readv/writev fails right away (true) or returns an error
-     * return value only in the callback (false) */
-    bool inject_immediately;
-} BlkdebugVars;
-
 typedef struct BDRVBlkdebugState {
-    BlkdebugVars vars;
-    QLIST_HEAD(list, BlkdebugRule) rules[BLKDBG_EVENT_MAX];
+    int state;
+    QLIST_HEAD(, BlkdebugRule) rules[BLKDBG_EVENT_MAX];
+    QSIMPLEQ_HEAD(, BlkdebugRule) active_rules;
 } BDRVBlkdebugState;
 
 typedef struct BlkdebugAIOCB {
@@ -79,6 +65,7 @@ typedef struct BlkdebugRule {
         } set_state;
     } options;
     QLIST_ENTRY(BlkdebugRule) next;
+    QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
 } BlkdebugRule;
 
 static QemuOptsList inject_error_opts = {
@@ -300,7 +287,7 @@ static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
     filename = c + 1;
 
     /* Set initial state */
-    s->vars.state = 1;
+    s->state = 1;
 
     /* Open the backing file */
     ret = bdrv_file_open(&bs->file, filename, flags);
@@ -326,18 +313,18 @@ static void blkdebug_aio_cancel(BlockDriverAIOCB *blockacb)
 }
 
 static BlockDriverAIOCB *inject_error(BlockDriverState *bs,
-    BlockDriverCompletionFunc *cb, void *opaque)
+    BlockDriverCompletionFunc *cb, void *opaque, BlkdebugRule *rule)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    int error = s->vars.inject_errno;
+    int error = rule->options.inject.error;
     struct BlkdebugAIOCB *acb;
     QEMUBH *bh;
 
-    if (s->vars.inject_once) {
-        s->vars.inject_errno = 0;
+    if (rule->options.inject.once) {
+        QSIMPLEQ_INIT(&s->active_rules);
     }
 
-    if (s->vars.inject_immediately) {
+    if (rule->options.inject.immediately) {
         return NULL;
     }
 
@@ -356,9 +343,10 @@ static BlockDriverAIOCB *blkdebug_aio_readv(BlockDriverState *bs,
     BlockDriverCompletionFunc *cb, void *opaque)
 {
     BDRVBlkdebugState *s = bs->opaque;
+    BlkdebugRule *rule = QSIMPLEQ_FIRST(&s->active_rules);
 
-    if (s->vars.inject_errno) {
-        return inject_error(bs, cb, opaque);
+    if (rule && rule->options.inject.error) {
+        return inject_error(bs, cb, opaque, rule);
     }
 
     return bdrv_aio_readv(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
@@ -369,9 +357,10 @@ static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
     BlockDriverCompletionFunc *cb, void *opaque)
 {
     BDRVBlkdebugState *s = bs->opaque;
+    BlkdebugRule *rule = QSIMPLEQ_FIRST(&s->active_rules);
 
-    if (s->vars.inject_errno) {
-        return inject_error(bs, cb, opaque);
+    if (rule && rule->options.inject.error) {
+        return inject_error(bs, cb, opaque, rule);
     }
 
     return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
@@ -391,41 +380,45 @@ static void blkdebug_close(BlockDriverState *bs)
     }
 }
 
-static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
-    BlkdebugVars *old_vars)
+static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
+    int old_state, bool injected)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugVars *vars = &s->vars;
 
     /* Only process rules for the current state */
-    if (rule->state && rule->state != old_vars->state) {
-        return;
+    if (rule->state && rule->state != old_state) {
+        return injected;
     }
 
     /* Take the action */
     switch (rule->action) {
     case ACTION_INJECT_ERROR:
-        vars->inject_errno       = rule->options.inject.error;
-        vars->inject_once        = rule->options.inject.once;
-        vars->inject_immediately = rule->options.inject.immediately;
+        if (!injected) {
+            QSIMPLEQ_INIT(&s->active_rules);
+            injected = true;
+        }
+        QSIMPLEQ_INSERT_HEAD(&s->active_rules, rule, active_next);
         break;
 
     case ACTION_SET_STATE:
-        vars->state              = rule->options.set_state.new_state;
+        s->state = rule->options.set_state.new_state;
         break;
     }
+    return injected;
 }
 
 static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event)
 {
     BDRVBlkdebugState *s = bs->opaque;
     struct BlkdebugRule *rule;
-    BlkdebugVars old_vars = s->vars;
+    int old_state = s->state;
+    bool injected;
 
     assert((int)event >= 0 && event < BLKDBG_EVENT_MAX);
 
+    injected = false;
     QLIST_FOREACH(rule, &s->rules[event], next) {
-        process_rule(bs, rule, &old_vars);
+        injected = process_rule(bs, rule, old_state, injected);
     }
 }
 
-- 
1.7.6.5

[Qemu-devel] [PATCH 14/25] blkdebug: optionally tie errors to a specific sector

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

This makes blkdebug scripts more powerful, and independent of the
exact sequence of operations performed by streaming.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index d12ebbf..59dcea0 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -59,6 +59,7 @@ typedef struct BlkdebugRule {
             int error;
             int immediately;
             int once;
+            int64_t sector;
         } inject;
         struct {
             int new_state;
@@ -85,6 +86,10 @@ static QemuOptsList inject_error_opts = {
             .type = QEMU_OPT_NUMBER,
         },
         {
+            .name = "sector",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
             .name = "once",
             .type = QEMU_OPT_BOOL,
         },
@@ -213,6 +218,7 @@ static int add_rule(QemuOpts *opts, void *opaque)
         rule->options.inject.once  = qemu_opt_get_bool(opts, "once", 0);
         rule->options.inject.immediately =
             qemu_opt_get_bool(opts, "immediately", 0);
+        rule->options.inject.sector = qemu_opt_get_number(opts, "sector", -1);
         break;
 
     case ACTION_SET_STATE:
@@ -343,7 +349,15 @@ static BlockDriverAIOCB *blkdebug_aio_readv(BlockDriverState *bs,
     BlockDriverCompletionFunc *cb, void *opaque)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugRule *rule = QSIMPLEQ_FIRST(&s->active_rules);
+    BlkdebugRule *rule = NULL;
+
+    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+        if (rule->options.inject.sector == -1 ||
+            (rule->options.inject.sector >= sector_num &&
+             rule->options.inject.sector < sector_num + nb_sectors)) {
+            break;
+        }
+    }
 
     if (rule && rule->options.inject.error) {
         return inject_error(bs, cb, opaque, rule);
@@ -357,7 +371,15 @@ static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
     BlockDriverCompletionFunc *cb, void *opaque)
 {
     BDRVBlkdebugState *s = bs->opaque;
-    BlkdebugRule *rule = QSIMPLEQ_FIRST(&s->active_rules);
+    BlkdebugRule *rule = NULL;
+
+    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+        if (rule->options.inject.sector == -1 ||
+            (rule->options.inject.sector >= sector_num &&
+             rule->options.inject.sector < sector_num + nb_sectors)) {
+            break;
+        }
+    }
 
     if (rule && rule->options.inject.error) {
         return inject_error(bs, cb, opaque, rule);
-- 
1.7.6.5

[Qemu-devel] [PATCH 15/25] raw: hook into blkdebug

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/raw.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block/raw.c b/block/raw.c
index 09d9b48..ff34ea4 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -12,12 +12,14 @@ static int raw_open(BlockDriverState *bs, int flags)
 static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
                                      int nb_sectors, QEMUIOVector *qiov)
 {
+    BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
     return bdrv_co_readv(bs->file, sector_num, nb_sectors, qiov);
 }
 
 static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
                                       int nb_sectors, QEMUIOVector *qiov)
 {
+    BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
     return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
 }
 
-- 
1.7.6.5

[Qemu-devel] [PATCH 16/25] block: copy over job and dirty bitmap fields in bdrv_append

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

While these should not be in use at the time a transaction is started,
a command in the prepare phase of a transaction might have added them,
so they need to be brought over.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 0acdcac..702821d 100644
--- a/block.c
+++ b/block.c
@@ -1027,6 +1027,16 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     tmp.iostatus_enabled  = bs_top->iostatus_enabled;
     tmp.iostatus          = bs_top->iostatus;
 
+    /* dirty bitmap */
+    tmp.dirty_count       = bs_top->dirty_count;
+    tmp.dirty_bitmap      = bs_top->dirty_bitmap;
+    assert(bs_new->dirty_bitmap == NULL);
+
+    /* job */
+    tmp.in_use            = bs_top->in_use;
+    tmp.job               = bs_top->job;
+    assert(bs_new->job == NULL);
+
     /* keep the same entry in bdrv_states */
     pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
     tmp.list = bs_top->list;
@@ -1051,6 +1061,11 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     /* clear the copied fields in the new backing file */
     bdrv_detach_dev(bs_new, bs_new->dev);
 
+    bs_new->job                = NULL;
+    bs_new->in_use             = 0;
+    bs_new->dirty_bitmap       = NULL;
+    bs_new->dirty_count        = 0;
+
     qemu_co_queue_init(&bs_new->throttled_reqs);
     memset(&bs_new->io_base,   0, sizeof(bs_new->io_base));
     memset(&bs_new->io_limits, 0, sizeof(bs_new->io_limits));
-- 
1.7.6.5

[Qemu-devel] [PATCH 17/25] block: introduce bdrv_swap, implement bdrv_append on top of it

[Kevin Wolf]

From: Paolo Bonzini <pbonzini@redhat.com>

The new function can be made a bit nicer than bdrv_append.  It swaps the
whole contents, and then swaps back (using the usual t=a;a=b;b=t idiom)
the fields that need to stay on top.  Thus, it does not need explicit
bdrv_detach_dev, bdrv_iostatus_disable, etc.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |  184 ++++++++++++++++++++++++++++++++++-----------------------------
 block.h |    1 +
 2 files changed, 100 insertions(+), 85 deletions(-)

diff --git a/block.c b/block.c
index 702821d..29857db 100644
--- a/block.c
+++ b/block.c
@@ -971,116 +971,130 @@ static void bdrv_rebind(BlockDriverState *bs)
     }
 }
 
-/*
- * Add new bs contents at the top of an image chain while the chain is
- * live, while keeping required fields on the top layer.
- *
- * This will modify the BlockDriverState fields, and swap contents
- * between bs_new and bs_top. Both bs_new and bs_top are modified.
- *
- * bs_new is required to be anonymous.
- *
- * This function does not create any image files.
- */
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
+static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
+                                     BlockDriverState *bs_src)
 {
-    BlockDriverState tmp;
-
-    /* bs_new must be anonymous */
-    assert(bs_new->device_name[0] == '\0');
-
-    tmp = *bs_new;
-
-    /* there are some fields that need to stay on the top layer: */
-    tmp.open_flags        = bs_top->open_flags;
+    /* move some fields that need to stay attached to the device */
+    bs_dest->open_flags         = bs_src->open_flags;
 
     /* dev info */
-    tmp.dev_ops           = bs_top->dev_ops;
-    tmp.dev_opaque        = bs_top->dev_opaque;
-    tmp.dev               = bs_top->dev;
-    tmp.buffer_alignment  = bs_top->buffer_alignment;
-    tmp.copy_on_read      = bs_top->copy_on_read;
+    bs_dest->dev_ops            = bs_src->dev_ops;
+    bs_dest->dev_opaque         = bs_src->dev_opaque;
+    bs_dest->dev                = bs_src->dev;
+    bs_dest->buffer_alignment   = bs_src->buffer_alignment;
+    bs_dest->copy_on_read       = bs_src->copy_on_read;
 
-    tmp.enable_write_cache = bs_top->enable_write_cache;
+    bs_dest->enable_write_cache = bs_src->enable_write_cache;
 
     /* i/o timing parameters */
-    tmp.slice_time        = bs_top->slice_time;
-    tmp.slice_start       = bs_top->slice_start;
-    tmp.slice_end         = bs_top->slice_end;
-    tmp.io_limits         = bs_top->io_limits;
-    tmp.io_base           = bs_top->io_base;
-    tmp.throttled_reqs    = bs_top->throttled_reqs;
-    tmp.block_timer       = bs_top->block_timer;
-    tmp.io_limits_enabled = bs_top->io_limits_enabled;
+    bs_dest->slice_time         = bs_src->slice_time;
+    bs_dest->slice_start        = bs_src->slice_start;
+    bs_dest->slice_end          = bs_src->slice_end;
+    bs_dest->io_limits          = bs_src->io_limits;
+    bs_dest->io_base            = bs_src->io_base;
+    bs_dest->throttled_reqs     = bs_src->throttled_reqs;
+    bs_dest->block_timer        = bs_src->block_timer;
+    bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
 
     /* geometry */
-    tmp.cyls              = bs_top->cyls;
-    tmp.heads             = bs_top->heads;
-    tmp.secs              = bs_top->secs;
-    tmp.translation       = bs_top->translation;
+    bs_dest->cyls               = bs_src->cyls;
+    bs_dest->heads              = bs_src->heads;
+    bs_dest->secs               = bs_src->secs;
+    bs_dest->translation        = bs_src->translation;
 
     /* r/w error */
-    tmp.on_read_error     = bs_top->on_read_error;
-    tmp.on_write_error    = bs_top->on_write_error;
+    bs_dest->on_read_error      = bs_src->on_read_error;
+    bs_dest->on_write_error     = bs_src->on_write_error;
 
     /* i/o status */
-    tmp.iostatus_enabled  = bs_top->iostatus_enabled;
-    tmp.iostatus          = bs_top->iostatus;
+    bs_dest->iostatus_enabled   = bs_src->iostatus_enabled;
+    bs_dest->iostatus           = bs_src->iostatus;
 
     /* dirty bitmap */
-    tmp.dirty_count       = bs_top->dirty_count;
-    tmp.dirty_bitmap      = bs_top->dirty_bitmap;
-    assert(bs_new->dirty_bitmap == NULL);
+    bs_dest->dirty_count        = bs_src->dirty_count;
+    bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
 
     /* job */
-    tmp.in_use            = bs_top->in_use;
-    tmp.job               = bs_top->job;
-    assert(bs_new->job == NULL);
+    bs_dest->in_use             = bs_src->in_use;
+    bs_dest->job                = bs_src->job;
 
     /* keep the same entry in bdrv_states */
-    pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
-    tmp.list = bs_top->list;
+    pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
+            bs_src->device_name);
+    bs_dest->list = bs_src->list;
+}
 
-    /* The contents of 'tmp' will become bs_top, as we are
-     * swapping bs_new and bs_top contents. */
-    tmp.backing_hd = bs_new;
-    pstrcpy(tmp.backing_file, sizeof(tmp.backing_file), bs_top->filename);
-    pstrcpy(tmp.backing_format, sizeof(tmp.backing_format),
-            bs_top->drv ? bs_top->drv->format_name : "");
-
-    /* swap contents of the fixed new bs and the current top */
-    *bs_new = *bs_top;
-    *bs_top = tmp;
-
-    /* device_name[] was carried over from the old bs_top.  bs_new
-     * shouldn't be in bdrv_states, so we need to make device_name[]
-     * reflect the anonymity of bs_new
-     */
-    bs_new->device_name[0] = '\0';
+/*
+ * Swap bs contents for two image chains while they are live,
+ * while keeping required fields on the BlockDriverState that is
+ * actually attached to a device.
+ *
+ * This will modify the BlockDriverState fields, and swap contents
+ * between bs_new and bs_old. Both bs_new and bs_old are modified.
+ *
+ * bs_new is required to be anonymous.
+ *
+ * This function does not create any image files.
+ */
+void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
+{
+    BlockDriverState tmp;
 
-    /* clear the copied fields in the new backing file */
-    bdrv_detach_dev(bs_new, bs_new->dev);
+    /* bs_new must be anonymous and shouldn't have anything fancy enabled */
+    assert(bs_new->device_name[0] == '\0');
+    assert(bs_new->dirty_bitmap == NULL);
+    assert(bs_new->job == NULL);
+    assert(bs_new->dev == NULL);
+    assert(bs_new->in_use == 0);
+    assert(bs_new->io_limits_enabled == false);
+    assert(bs_new->block_timer == NULL);
 
-    bs_new->job                = NULL;
-    bs_new->in_use             = 0;
-    bs_new->dirty_bitmap       = NULL;
-    bs_new->dirty_count        = 0;
+    tmp = *bs_new;
+    *bs_new = *bs_old;
+    *bs_old = tmp;
 
-    qemu_co_queue_init(&bs_new->throttled_reqs);
-    memset(&bs_new->io_base,   0, sizeof(bs_new->io_base));
-    memset(&bs_new->io_limits, 0, sizeof(bs_new->io_limits));
-    bdrv_iostatus_disable(bs_new);
+    /* there are some fields that should not be swapped, move them back */
+    bdrv_move_feature_fields(&tmp, bs_old);
+    bdrv_move_feature_fields(bs_old, bs_new);
+    bdrv_move_feature_fields(bs_new, &tmp);
 
-    /* we don't use bdrv_io_limits_disable() for this, because we don't want
-     * to affect or delete the block_timer, as it has been moved to bs_top */
-    bs_new->io_limits_enabled = false;
-    bs_new->block_timer       = NULL;
-    bs_new->slice_time        = 0;
-    bs_new->slice_start       = 0;
-    bs_new->slice_end         = 0;
+    /* bs_new shouldn't be in bdrv_states even after the swap!  */
+    assert(bs_new->device_name[0] == '\0');
+
+    /* Check a few fields that should remain attached to the device */
+    assert(bs_new->dev == NULL);
+    assert(bs_new->job == NULL);
+    assert(bs_new->in_use == 0);
+    assert(bs_new->io_limits_enabled == false);
+    assert(bs_new->block_timer == NULL);
 
     bdrv_rebind(bs_new);
-    bdrv_rebind(bs_top);
+    bdrv_rebind(bs_old);
+}
+
+/*
+ * Add new bs contents at the top of an image chain while the chain is
+ * live, while keeping required fields on the top layer.
+ *
+ * This will modify the BlockDriverState fields, and swap contents
+ * between bs_new and bs_top. Both bs_new and bs_top are modified.
+ *
+ * bs_new is required to be anonymous.
+ *
+ * This function does not create any image files.
+ */
+void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
+{
+    bdrv_swap(bs_new, bs_top);
+
+    /* The contents of 'tmp' will become bs_top, as we are
+     * swapping bs_new and bs_top contents. */
+    bs_top->backing_hd = bs_new;
+    bs_top->open_flags &= ~BDRV_O_NO_BACKING;
+    pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
+            bs_new->filename);
+    pstrcpy(bs_top->backing_format, sizeof(bs_top->backing_format),
+            bs_new->drv ? bs_new->drv->format_name : "");
 }
 
 void bdrv_delete(BlockDriverState *bs)
diff --git a/block.h b/block.h
index 42e30d6..3af93c6 100644
--- a/block.h
+++ b/block.h
@@ -122,6 +122,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
 BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
+void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
-- 
1.7.6.5

[Qemu-devel] [PATCH 18/25] fdc: rewrite seek and DSKCHG bit handling

[Kevin Wolf]

From: Pavel Hrdina <phrdina@redhat.com>

This bit is cleared on every successful seek to a different track (cylinder).
The seek is also called on revalidate or on read/write/format commands which
also clear the DSKCHG bit.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/fdc.c |   79 ++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 5b3224b..0270264 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -153,8 +153,12 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
         }
 #endif
         drv->head = head;
-        if (drv->track != track)
+        if (drv->track != track) {
+            if (drv->bs != NULL && bdrv_is_inserted(drv->bs)) {
+                drv->media_changed = 0;
+            }
             ret = 1;
+        }
         drv->track = track;
         drv->sect = sect;
     }
@@ -170,9 +174,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
 static void fd_recalibrate(FDrive *drv)
 {
     FLOPPY_DPRINTF("recalibrate\n");
-    drv->head = 0;
-    drv->track = 0;
-    drv->sect = 1;
+    fd_seek(drv, 0, 0, 1, 1);
 }
 
 /* Revalidate a disk drive after a disk change */
@@ -711,14 +713,6 @@ static void fdctrl_raise_irq(FDCtrl *fdctrl, uint8_t status0)
         qemu_set_irq(fdctrl->irq, 1);
         fdctrl->sra |= FD_SRA_INTPEND;
     }
-    if (status0 & FD_SR0_SEEK) {
-        FDrive *cur_drv;
-        /* A seek clears the disk change line (if a disk is inserted) */
-        cur_drv = get_cur_drv(fdctrl);
-        if (cur_drv->bs != NULL && bdrv_is_inserted(cur_drv->bs)) {
-            cur_drv->media_changed = 0;
-        }
-    }
 
     fdctrl->reset_sensei = 0;
     fdctrl->status0 = status0;
@@ -997,7 +991,10 @@ static void fdctrl_unimplemented(FDCtrl *fdctrl, int direction)
     fdctrl_set_fifo(fdctrl, 1, 0);
 }
 
-/* Seek to next sector */
+/* Seek to next sector
+ * returns 0 when end of track reached (for DBL_SIDES on head 1)
+ * otherwise returns 1
+ */
 static int fdctrl_seek_to_next_sect(FDCtrl *fdctrl, FDrive *cur_drv)
 {
     FLOPPY_DPRINTF("seek to next sector (%d %02x %02x => %d)\n",
@@ -1005,30 +1002,39 @@ static int fdctrl_seek_to_next_sect(FDCtrl *fdctrl, FDrive *cur_drv)
                    fd_sector(cur_drv));
     /* XXX: cur_drv->sect >= cur_drv->last_sect should be an
        error in fact */
-    if (cur_drv->sect >= cur_drv->last_sect ||
-        cur_drv->sect == fdctrl->eot) {
-        cur_drv->sect = 1;
+    uint8_t new_head = cur_drv->head;
+    uint8_t new_track = cur_drv->track;
+    uint8_t new_sect = cur_drv->sect;
+
+    int ret = 1;
+
+    if (new_sect >= cur_drv->last_sect ||
+        new_sect == fdctrl->eot) {
+        new_sect = 1;
         if (FD_MULTI_TRACK(fdctrl->data_state)) {
-            if (cur_drv->head == 0 &&
+            if (new_head == 0 &&
                 (cur_drv->flags & FDISK_DBL_SIDES) != 0) {
-                cur_drv->head = 1;
+                new_head = 1;
             } else {
-                cur_drv->head = 0;
-                cur_drv->track++;
-                if ((cur_drv->flags & FDISK_DBL_SIDES) == 0)
-                    return 0;
+                new_head = 0;
+                new_track++;
+                if ((cur_drv->flags & FDISK_DBL_SIDES) == 0) {
+                    ret = 0;
+                }
             }
         } else {
-            cur_drv->track++;
-            return 0;
+            new_track++;
+            ret = 0;
+        }
+        if (ret == 1) {
+            FLOPPY_DPRINTF("seek to next track (%d %02x %02x => %d)\n",
+                    new_head, new_track, new_sect, fd_sector(cur_drv));
         }
-        FLOPPY_DPRINTF("seek to next track (%d %02x %02x => %d)\n",
-                       cur_drv->head, cur_drv->track,
-                       cur_drv->sect, fd_sector(cur_drv));
     } else {
-        cur_drv->sect++;
+        new_sect++;
     }
-    return 1;
+    fd_seek(cur_drv, new_head, new_track, new_sect, 1);
+    return ret;
 }
 
 /* Callback for transfer end (stop or abort) */
@@ -1626,11 +1632,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
     /* The seek command just sends step pulses to the drive and doesn't care if
      * there is a medium inserted of if it's banging the head against the drive.
      */
-    if (fdctrl->fifo[2] > cur_drv->max_track) {
-        cur_drv->track = cur_drv->max_track;
-    } else {
-        cur_drv->track = fdctrl->fifo[2];
-    }
+    fd_seek(cur_drv, cur_drv->head, fdctrl->fifo[2], cur_drv->sect, 1);
     /* Raise Interrupt */
     fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
 }
@@ -1695,9 +1697,10 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
     if (fdctrl->fifo[2] + cur_drv->track >= cur_drv->max_track) {
-        cur_drv->track = cur_drv->max_track - 1;
+        fd_seek(cur_drv, cur_drv->head, cur_drv->max_track - 1,
+                cur_drv->sect, 1);
     } else {
-        cur_drv->track += fdctrl->fifo[2];
+        fd_seek(cur_drv, cur_drv->head, fdctrl->fifo[2], cur_drv->sect, 1);
     }
     fdctrl_reset_fifo(fdctrl);
     /* Raise Interrupt */
@@ -1711,9 +1714,9 @@ static void fdctrl_handle_relative_seek_in(FDCtrl *fdctrl, int direction)
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
     if (fdctrl->fifo[2] > cur_drv->track) {
-        cur_drv->track = 0;
+        fd_seek(cur_drv, cur_drv->head, 0, cur_drv->sect, 1);
     } else {
-        cur_drv->track -= fdctrl->fifo[2];
+        fd_seek(cur_drv, cur_drv->head, fdctrl->fifo[2], cur_drv->sect, 1);
     }
     fdctrl_reset_fifo(fdctrl);
     /* Raise Interrupt */
-- 
1.7.6.5

[Qemu-devel] [PATCH 19/25] fdc: fix interrupt handling

[Kevin Wolf]

From: Pavel Hrdina <phrdina@redhat.com>

If you call the SENSE INTERRUPT STATUS command while there is no interrupt
waiting you get as result unknown command.

Fixed status0 register handling for read/write/format commands.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/fdc.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 0270264..e28841c 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -307,6 +307,9 @@ enum {
 };
 
 enum {
+    FD_SR0_DS0      = 0x01,
+    FD_SR0_DS1      = 0x02,
+    FD_SR0_HEAD     = 0x04,
     FD_SR0_EQPMT    = 0x10,
     FD_SR0_SEEK     = 0x20,
     FD_SR0_ABNTERM  = 0x40,
@@ -972,14 +975,15 @@ static void fdctrl_reset_fifo(FDCtrl *fdctrl)
 }
 
 /* Set FIFO status for the host to read */
-static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len, int do_irq)
+static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len, uint8_t status0)
 {
     fdctrl->data_dir = FD_DIR_READ;
     fdctrl->data_len = fifo_len;
     fdctrl->data_pos = 0;
     fdctrl->msr |= FD_MSR_CMDBUSY | FD_MSR_RQM | FD_MSR_DIO;
-    if (do_irq)
-        fdctrl_raise_irq(fdctrl, 0x00);
+    if (status0) {
+        fdctrl_raise_irq(fdctrl, status0);
+    }
 }
 
 /* Set an error: unimplemented/unknown command */
@@ -1044,10 +1048,12 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     FDrive *cur_drv;
 
     cur_drv = get_cur_drv(fdctrl);
+    fdctrl->status0 = status0 | FD_SR0_SEEK | (cur_drv->head << 2) |
+                      GET_CUR_DRV(fdctrl);
+
     FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
-                   status0, status1, status2,
-                   status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl));
-    fdctrl->fifo[0] = status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
+                   status0, status1, status2, fdctrl->status0);
+    fdctrl->fifo[0] = fdctrl->status0;
     fdctrl->fifo[1] = status1;
     fdctrl->fifo[2] = status2;
     fdctrl->fifo[3] = cur_drv->track;
@@ -1060,7 +1066,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     }
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
     fdctrl->msr &= ~FD_MSR_NONDMA;
-    fdctrl_set_fifo(fdctrl, 7, 1);
+    fdctrl_set_fifo(fdctrl, 7, fdctrl->status0);
 }
 
 /* Prepare a data transfer (either DMA or FIFO) */
@@ -1175,7 +1181,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */
-    fdctrl_raise_irq(fdctrl, 0x00);
+    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
 
     return;
 }
@@ -1604,16 +1610,18 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
 {
     FDrive *cur_drv = get_cur_drv(fdctrl);
 
-    if(fdctrl->reset_sensei > 0) {
+    if (fdctrl->reset_sensei > 0) {
         fdctrl->fifo[0] =
             FD_SR0_RDYCHG + FD_RESET_SENSEI_COUNT - fdctrl->reset_sensei;
         fdctrl->reset_sensei--;
+    } else if (!(fdctrl->sra & FD_SRA_INTPEND)) {
+        fdctrl->fifo[0] = FD_SR0_INVCMD;
+        fdctrl_set_fifo(fdctrl, 1, 0);
+        return;
     } else {
-        /* XXX: status0 handling is broken for read/write
-           commands, so we do this hack. It should be suppressed
-           ASAP */
         fdctrl->fifo[0] =
-            FD_SR0_SEEK | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
+                (fdctrl->status0 & ~(FD_SR0_HEAD | FD_SR0_DS1 | FD_SR0_DS0))
+                | GET_CUR_DRV(fdctrl);
     }
 
     fdctrl->fifo[1] = cur_drv->track;
-- 
1.7.6.5

[Qemu-devel] [PATCH 20/25] fdc_test: update media_change test

[Kevin Wolf]

From: Pavel Hrdina <phrdina@redhat.com>

After rewrite DSKCHG bit handling the test has to be updated. Now
is needed to seek to different track to clear DSKCHG bit.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/fdc-test.c |   25 +++++++++++++++++--------
 1 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 610e2f1..5f52f6c 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -156,19 +156,16 @@ static uint8_t send_read_command(void)
     return ret;
 }
 
-static void send_step_pulse(void)
+static void send_step_pulse(int cyl)
 {
     int drive = 0;
     int head = 0;
-    static int cyl = 0;
 
     floppy_send(CMD_SEEK);
     floppy_send(head << 2 | drive);
     g_assert(!get_irq(FLOPPY_IRQ));
     floppy_send(cyl);
     ack_irq();
-
-    cyl = (cyl + 1) % 4;
 }
 
 static uint8_t cmos_read(uint8_t reg)
@@ -195,8 +192,7 @@ static void test_no_media_on_start(void)
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
-    send_step_pulse();
-    send_step_pulse();
+    send_step_pulse(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
@@ -227,7 +223,14 @@ static void test_media_change(void)
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
-    send_step_pulse();
+    send_step_pulse(0);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+
+    /* Step to next track should clear DSKCHG bit. */
+    send_step_pulse(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_clear(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
@@ -243,7 +246,13 @@ static void test_media_change(void)
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
 
-    send_step_pulse();
+    send_step_pulse(0);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+    dir = inb(FLOPPY_BASE + reg_dir);
+    assert_bit_set(dir, DSKCHG);
+
+    send_step_pulse(1);
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
     dir = inb(FLOPPY_BASE + reg_dir);
-- 
1.7.6.5

[Qemu-devel] [PATCH 21/25] fdc_test: introduce test_sense_interrupt

[Kevin Wolf]

From: Pavel Hrdina <phrdina@redhat.com>

Calling sense interrupt status while there is no interrupt should
return invalid command (0x80).

Read command should always returns in st0 seek_end bit set to 1.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/fdc-test.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 5f52f6c..585fb0e 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -142,7 +142,7 @@ static uint8_t send_read_command(void)
     }
 
     st0 = floppy_recv();
-    if (st0 != 0x40) {
+    if (st0 != 0x60) {
         ret = 1;
     }
 
@@ -259,6 +259,28 @@ static void test_media_change(void)
     assert_bit_set(dir, DSKCHG);
 }
 
+static void test_sense_interrupt(void)
+{
+    int drive = 0;
+    int head = 0;
+    int cyl = 0;
+    int ret = 0;
+
+    floppy_send(CMD_SENSE_INT);
+    ret = floppy_recv();
+    g_assert(ret == 0x80);
+
+    floppy_send(CMD_SEEK);
+    floppy_send(head << 2 | drive);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(cyl);
+
+    floppy_send(CMD_SENSE_INT);
+    ret = floppy_recv();
+    g_assert(ret == 0x20);
+    floppy_recv();
+}
+
 /* success if no crash or abort */
 static void fuzz_registers(void)
 {
@@ -306,6 +328,7 @@ int main(int argc, char **argv)
     qtest_add_func("/fdc/no_media_on_start", test_no_media_on_start);
     qtest_add_func("/fdc/read_without_media", test_read_without_media);
     qtest_add_func("/fdc/media_change", test_media_change);
+    qtest_add_func("/fdc/sense_interrupt", test_sense_interrupt);
     qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
 
     ret = g_test_run();
-- 
1.7.6.5

[Qemu-devel] [PATCH 22/25] fdc: Drop broken code for user-defined floppy geometry

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

bdrv_get_floppy_geometry_hint() fails to store through its parameter
drive when bs has a geometry hint.  Makes fd_revalidate() assign
random crap to drv->drive.

Has been broken that way for ages.  Harmless, because:

* The only way to set a geometry hint is -drive if=none,cyls=...
  Since commit c219331e, probably unintentional.

* The only use of drv->drive is as argument to another
  bdrv_get_floppy_geometry_hint().  Which doesn't use it, since the
  geometry hint is still there.

Drop the broken code, ignore -drive parameter cyls, heads and secs for
floppies even with if=none, just like before commit c219331e.  Matches
-help, which explains cyls, heads, secs as "hard disk physical
geometry".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c  |   62 ++++++++++++++++++++++++++++----------------------------------
 hw/fdc.c |    3 ---
 2 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 29857db..f2540b9 100644
--- a/block.c
+++ b/block.c
@@ -2337,46 +2337,40 @@ void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
     uint64_t nb_sectors, size;
     int i, first_match, match;
 
-    bdrv_get_geometry_hint(bs, nb_heads, max_track, last_sect);
-    if (*nb_heads != 0 && *max_track != 0 && *last_sect != 0) {
-        /* User defined disk */
-        *rate = FDRIVE_RATE_500K;
-    } else {
-        bdrv_get_geometry(bs, &nb_sectors);
-        match = -1;
-        first_match = -1;
-        for (i = 0; ; i++) {
-            parse = &fd_formats[i];
-            if (parse->drive == FDRIVE_DRV_NONE) {
+    bdrv_get_geometry(bs, &nb_sectors);
+    match = -1;
+    first_match = -1;
+    for (i = 0; ; i++) {
+        parse = &fd_formats[i];
+        if (parse->drive == FDRIVE_DRV_NONE) {
+            break;
+        }
+        if (drive_in == parse->drive ||
+            drive_in == FDRIVE_DRV_NONE) {
+            size = (parse->max_head + 1) * parse->max_track *
+                parse->last_sect;
+            if (nb_sectors == size) {
+                match = i;
                 break;
             }
-            if (drive_in == parse->drive ||
-                drive_in == FDRIVE_DRV_NONE) {
-                size = (parse->max_head + 1) * parse->max_track *
-                    parse->last_sect;
-                if (nb_sectors == size) {
-                    match = i;
-                    break;
-                }
-                if (first_match == -1) {
-                    first_match = i;
-                }
-            }
-        }
-        if (match == -1) {
             if (first_match == -1) {
-                match = 1;
-            } else {
-                match = first_match;
+                first_match = i;
             }
-            parse = &fd_formats[match];
         }
-        *nb_heads = parse->max_head + 1;
-        *max_track = parse->max_track;
-        *last_sect = parse->last_sect;
-        *drive = parse->drive;
-        *rate = parse->rate;
     }
+    if (match == -1) {
+        if (first_match == -1) {
+            match = 1;
+        } else {
+            match = first_match;
+        }
+        parse = &fd_formats[match];
+    }
+    *nb_heads = parse->max_head + 1;
+    *max_track = parse->max_track;
+    *last_sect = parse->last_sect;
+    *drive = parse->drive;
+    *rate = parse->rate;
 }
 
 int bdrv_get_translation_hint(BlockDriverState *bs)
diff --git a/hw/fdc.c b/hw/fdc.c
index e28841c..edf0706 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -191,9 +191,6 @@ static void fd_revalidate(FDrive *drv)
                                       &last_sect, drv->drive, &drive, &rate);
         if (!bdrv_is_inserted(drv->bs)) {
             FLOPPY_DPRINTF("No disk in drive\n");
-        } else if (nb_heads != 0 && max_track != 0 && last_sect != 0) {
-            FLOPPY_DPRINTF("User defined disk (%d %d %d)\n",
-                           nb_heads - 1, max_track, last_sect);
         } else {
             FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
                            max_track, last_sect, ro ? "ro" : "rw");
-- 
1.7.6.5

[Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

Commit 5bbdbb46 moved it to block.c because "other geometry guessing
functions already reside in block.c".  Device-specific functionality
should be kept in device code, not the block layer.  Move it back.

Disk geometry guessing is still in block.c.  To be moved out in a
later patch series.

Bonus: the floppy type used in pc_cmos_init() now obviously matches
the one in the FDrive.  Before, we relied on
bdrv_get_floppy_geometry_hint() picking the same type both in
fd_revalidate() and in pc_cmos_init().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c  |  101 ---------------------------------------------------
 block.h  |   18 ---------
 hw/fdc.c |  122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/fdc.h |   10 +++++-
 hw/pc.c  |   13 ++-----
 5 files changed, 124 insertions(+), 140 deletions(-)

diff --git a/block.c b/block.c
index f2540b9..fa1789c 100644
--- a/block.c
+++ b/block.c
@@ -2272,107 +2272,6 @@ void bdrv_set_io_limits(BlockDriverState *bs,
     bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
 }
 
-/* Recognize floppy formats */
-typedef struct FDFormat {
-    FDriveType drive;
-    uint8_t last_sect;
-    uint8_t max_track;
-    uint8_t max_head;
-    FDriveRate rate;
-} FDFormat;
-
-static const FDFormat fd_formats[] = {
-    /* First entry is default format */
-    /* 1.44 MB 3"1/2 floppy disks */
-    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
-    /* 2.88 MB 3"1/2 floppy disks */
-    { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, },
-    { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, },
-    { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, },
-    { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, },
-    { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, },
-    /* 720 kB 3"1/2 floppy disks */
-    { FDRIVE_DRV_144,  9, 80, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, },
-    /* 1.2 MB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, },
-    /* 720 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, },
-    /* 360 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  9, 40, 1, FDRIVE_RATE_300K, },
-    { FDRIVE_DRV_120,  9, 40, 0, FDRIVE_RATE_300K, },
-    { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, },
-    { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, },
-    /* 320 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  8, 40, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_120,  8, 40, 0, FDRIVE_RATE_250K, },
-    /* 360 kB must match 5"1/4 better than 3"1/2... */
-    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
-    /* end */
-    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
-};
-
-void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
-                                   int *max_track, int *last_sect,
-                                   FDriveType drive_in, FDriveType *drive,
-                                   FDriveRate *rate)
-{
-    const FDFormat *parse;
-    uint64_t nb_sectors, size;
-    int i, first_match, match;
-
-    bdrv_get_geometry(bs, &nb_sectors);
-    match = -1;
-    first_match = -1;
-    for (i = 0; ; i++) {
-        parse = &fd_formats[i];
-        if (parse->drive == FDRIVE_DRV_NONE) {
-            break;
-        }
-        if (drive_in == parse->drive ||
-            drive_in == FDRIVE_DRV_NONE) {
-            size = (parse->max_head + 1) * parse->max_track *
-                parse->last_sect;
-            if (nb_sectors == size) {
-                match = i;
-                break;
-            }
-            if (first_match == -1) {
-                first_match = i;
-            }
-        }
-    }
-    if (match == -1) {
-        if (first_match == -1) {
-            match = 1;
-        } else {
-            match = first_match;
-        }
-        parse = &fd_formats[match];
-    }
-    *nb_heads = parse->max_head + 1;
-    *max_track = parse->max_track;
-    *last_sect = parse->last_sect;
-    *drive = parse->drive;
-    *rate = parse->rate;
-}
-
 int bdrv_get_translation_hint(BlockDriverState *bs)
 {
     return bs->translation;
diff --git a/block.h b/block.h
index 3af93c6..c1c7500 100644
--- a/block.h
+++ b/block.h
@@ -267,24 +267,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs,
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs);
-typedef enum FDriveType {
-    FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
-    FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
-    FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
-    FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
-} FDriveType;
-
-typedef enum FDriveRate {
-    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
-    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
-    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
-    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
-} FDriveRate;
-
-void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
-                                   int *max_track, int *last_sect,
-                                   FDriveType drive_in, FDriveType *drive,
-                                   FDriveRate *rate);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
diff --git a/hw/fdc.c b/hw/fdc.c
index edf0706..41191c7 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -52,6 +52,113 @@
 /********************************************************/
 /* Floppy drive emulation                               */
 
+typedef enum FDriveRate {
+    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
+    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
+    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
+    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
+} FDriveRate;
+
+typedef struct FDFormat {
+    FDriveType drive;
+    uint8_t last_sect;
+    uint8_t max_track;
+    uint8_t max_head;
+    FDriveRate rate;
+} FDFormat;
+
+static const FDFormat fd_formats[] = {
+    /* First entry is default format */
+    /* 1.44 MB 3"1/2 floppy disks */
+    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
+    /* 2.88 MB 3"1/2 floppy disks */
+    { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, },
+    /* 720 kB 3"1/2 floppy disks */
+    { FDRIVE_DRV_144,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, },
+    /* 1.2 MB 5"1/4 floppy disks */
+    { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, },
+    /* 720 kB 5"1/4 floppy disks */
+    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, },
+    /* 360 kB 5"1/4 floppy disks */
+    { FDRIVE_DRV_120,  9, 40, 1, FDRIVE_RATE_300K, },
+    { FDRIVE_DRV_120,  9, 40, 0, FDRIVE_RATE_300K, },
+    { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, },
+    { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, },
+    /* 320 kB 5"1/4 floppy disks */
+    { FDRIVE_DRV_120,  8, 40, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_120,  8, 40, 0, FDRIVE_RATE_250K, },
+    /* 360 kB must match 5"1/4 better than 3"1/2... */
+    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
+    /* end */
+    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
+};
+
+static void pick_geometry(BlockDriverState *bs, int *nb_heads,
+                          int *max_track, int *last_sect,
+                          FDriveType drive_in, FDriveType *drive,
+                          FDriveRate *rate)
+{
+    const FDFormat *parse;
+    uint64_t nb_sectors, size;
+    int i, first_match, match;
+
+    bdrv_get_geometry(bs, &nb_sectors);
+    match = -1;
+    first_match = -1;
+    for (i = 0; ; i++) {
+        parse = &fd_formats[i];
+        if (parse->drive == FDRIVE_DRV_NONE) {
+            break;
+        }
+        if (drive_in == parse->drive ||
+            drive_in == FDRIVE_DRV_NONE) {
+            size = (parse->max_head + 1) * parse->max_track *
+                parse->last_sect;
+            if (nb_sectors == size) {
+                match = i;
+                break;
+            }
+            if (first_match == -1) {
+                first_match = i;
+            }
+        }
+    }
+    if (match == -1) {
+        if (first_match == -1) {
+            match = 1;
+        } else {
+            match = first_match;
+        }
+        parse = &fd_formats[match];
+    }
+    *nb_heads = parse->max_head + 1;
+    *max_track = parse->max_track;
+    *last_sect = parse->last_sect;
+    *drive = parse->drive;
+    *rate = parse->rate;
+}
+
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
 #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
 
@@ -187,8 +294,8 @@ static void fd_revalidate(FDrive *drv)
     FLOPPY_DPRINTF("revalidate\n");
     if (drv->bs != NULL) {
         ro = bdrv_is_read_only(drv->bs);
-        bdrv_get_floppy_geometry_hint(drv->bs, &nb_heads, &max_track,
-                                      &last_sect, drv->drive, &drive, &rate);
+        pick_geometry(drv->bs, &nb_heads, &max_track,
+                      &last_sect, drv->drive, &drive, &rate);
         if (!bdrv_is_inserted(drv->bs)) {
             FLOPPY_DPRINTF("No disk in drive\n");
         } else {
@@ -2054,18 +2161,13 @@ static int sun4m_fdc_init1(SysBusDevice *dev)
     return fdctrl_init_common(fdctrl);
 }
 
-void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev)
+FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
 {
-    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, dev);
-    FDCtrl *fdctrl = &isa->state;
-    int i;
+    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, fdc);
 
-    for (i = 0; i < MAX_FD; i++) {
-        bs[i] = fdctrl->drives[i].bs;
-    }
+    return isa->state.drives[i].drive;
 }
 
-
 static const VMStateDescription vmstate_isa_fdc ={
     .name = "fdc",
     .version_id = 2,
diff --git a/hw/fdc.h b/hw/fdc.h
index 1b32b17..b5c9f31 100644
--- a/hw/fdc.h
+++ b/hw/fdc.h
@@ -6,11 +6,19 @@
 /* fdc.c */
 #define MAX_FD 2
 
+typedef enum FDriveType {
+    FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
+    FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
+    FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
+    FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
+} FDriveType;
+
 ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
 void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
                         target_phys_addr_t mmio_base, DriveInfo **fds);
 void sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base,
                        DriveInfo **fds, qemu_irq *fdc_tc);
-void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev);
+
+FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
 
 #endif
diff --git a/hw/pc.c b/hw/pc.c
index c7e9ab3..e5e7647 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
                   ISADevice *floppy, BusState *idebus0, BusState *idebus1,
                   ISADevice *s)
 {
-    int val, nb, nb_heads, max_track, last_sect, i;
-    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
-    FDriveRate rate;
-    BlockDriverState *fd[MAX_FD];
+    int val, nb, i;
+    FDriveType fd_type[2];
     static pc_cmos_init_late_arg arg;
 
     /* various important CMOS locations needed by PC/Bochs bios */
@@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
 
     /* floppy type */
     if (floppy) {
-        fdc_get_bs(fd, floppy);
         for (i = 0; i < 2; i++) {
-            if (fd[i]) {
-                bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
-                                              &last_sect, FDRIVE_DRV_NONE,
-                                              &fd_type[i], &rate);
-            }
+            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
         }
     }
     val = (cmos_get_fd_drive_type(fd_type[0]) << 4) |
-- 
1.7.6.5

[Qemu-devel] [PATCH 24/25] qtest: Tidy up temporary files properly

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

Each test litters /tmp with several files: a pid file and two
sockets.  Tidy up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/libqtest.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 071b6be..02d0392 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -40,6 +40,7 @@ struct QTestState
     bool irq_level[MAX_IRQ];
     GString *rx;
     gchar *pid_file;
+    char *socket_path, *qmp_socket_path;
 };
 
 #define g_assert_no_errno(ret) do { \
@@ -88,8 +89,6 @@ QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, ret, i;
-    gchar *socket_path;
-    gchar *qmp_socket_path;
     gchar *pid_file;
     gchar *command;
     const char *qemu_binary;
@@ -98,14 +97,14 @@ QTestState *qtest_init(const char *extra_args)
     qemu_binary = getenv("QTEST_QEMU_BINARY");
     g_assert(qemu_binary != NULL);
 
-    socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
-    qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
-    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
-
     s = g_malloc(sizeof(*s));
 
-    sock = init_socket(socket_path);
-    qmpsock = init_socket(qmp_socket_path);
+    s->socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
+    s->qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
+    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
+
+    sock = init_socket(s->socket_path);
+    qmpsock = init_socket(s->qmp_socket_path);
 
     pid = fork();
     if (pid == 0) {
@@ -115,8 +114,8 @@ QTestState *qtest_init(const char *extra_args)
                                   "-qmp unix:%s,nowait "
                                   "-pidfile %s "
                                   "-machine accel=qtest "
-                                  "%s", qemu_binary, socket_path,
-                                  qmp_socket_path, pid_file,
+                                  "%s", qemu_binary, s->socket_path,
+                                  s->qmp_socket_path, pid_file,
                                   extra_args ?: "");
 
         ret = system(command);
@@ -133,9 +132,6 @@ QTestState *qtest_init(const char *extra_args)
         s->irq_level[i] = false;
     }
 
-    g_free(socket_path);
-    g_free(qmp_socket_path);
-
     /* Read the QMP greeting and then do the handshake */
     qtest_qmp(s, "");
     qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }");
@@ -160,6 +156,13 @@ void qtest_quit(QTestState *s)
 
         fclose(f);
     }
+
+    unlink(s->pid_file);
+    unlink(s->socket_path);
+    unlink(s->qmp_socket_path);
+    g_free(s->pid_file);
+    g_free(s->socket_path);
+    g_free(s->qmp_socket_path);
 }
 
 static void socket_sendf(int fd, const char *fmt, va_list ap)
-- 
1.7.6.5

[Qemu-devel] [PATCH 25/25] block: Factor bdrv_read_unthrottled() out of guess_disk_lchs()

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

To prepare move of guess_disk_lchs() into hw/, where it poking
BlockDriverState member io_limits_enabled directly would be unclean.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |   24 +++++++++++++++++-------
 block.h |    2 ++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index fa1789c..ba3162c 100644
--- a/block.c
+++ b/block.c
@@ -1639,6 +1639,20 @@ int bdrv_read(BlockDriverState *bs, int64_t sector_num,
     return bdrv_rw_co(bs, sector_num, buf, nb_sectors, false);
 }
 
+/* Just like bdrv_read(), but with I/O throttling temporarily disabled */
+int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
+                          uint8_t *buf, int nb_sectors)
+{
+    bool enabled;
+    int ret;
+
+    enabled = bs->io_limits_enabled;
+    bs->io_limits_enabled = false;
+    ret = bdrv_read(bs, 0, buf, 1);
+    bs->io_limits_enabled = enabled;
+    return ret;
+}
+
 #define BITS_PER_LONG  (sizeof(unsigned long) * 8)
 
 static void set_dirty_bitmap(BlockDriverState *bs, int64_t sector_num,
@@ -2136,11 +2150,10 @@ static int guess_disk_lchs(BlockDriverState *bs,
                            int *pcylinders, int *pheads, int *psectors)
 {
     uint8_t buf[BDRV_SECTOR_SIZE];
-    int ret, i, heads, sectors, cylinders;
+    int i, heads, sectors, cylinders;
     struct partition *p;
     uint32_t nr_sects;
     uint64_t nb_sectors;
-    bool enabled;
 
     bdrv_get_geometry(bs, &nb_sectors);
 
@@ -2149,12 +2162,9 @@ static int guess_disk_lchs(BlockDriverState *bs,
      * but also in async I/O mode. So the I/O throttling function has to
      * be disabled temporarily here, not permanently.
      */
-    enabled = bs->io_limits_enabled;
-    bs->io_limits_enabled = false;
-    ret = bdrv_read(bs, 0, buf, 1);
-    bs->io_limits_enabled = enabled;
-    if (ret < 0)
+    if (bdrv_read_unthrottled(bs, 0, buf, 1) < 0) {
         return -1;
+    }
     /* test msdos magic */
     if (buf[510] != 0x55 || buf[511] != 0xaa)
         return -1;
diff --git a/block.h b/block.h
index c1c7500..b24f664 100644
--- a/block.h
+++ b/block.h
@@ -142,6 +142,8 @@ bool bdrv_dev_is_tray_open(BlockDriverState *bs);
 bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
               uint8_t *buf, int nb_sectors);
+int bdrv_read_unthrottled(BlockDriverState *bs, int64_t sector_num,
+                          uint8_t *buf, int nb_sectors);
 int bdrv_write(BlockDriverState *bs, int64_t sector_num,
                const uint8_t *buf, int nb_sectors);
 int bdrv_pread(BlockDriverState *bs, int64_t offset,
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Anthony Liguori]

On 07/09/2012 09:16 AM, Kevin Wolf wrote:
> From: Markus Armbruster<armbru@redhat.com>
>
> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
> functions already reside in block.c".  Device-specific functionality
> should be kept in device code, not the block layer.  Move it back.
>
> Disk geometry guessing is still in block.c.  To be moved out in a
> later patch series.
>
> Bonus: the floppy type used in pc_cmos_init() now obviously matches
> the one in the FDrive.  Before, we relied on
> bdrv_get_floppy_geometry_hint() picking the same type both in
> fd_revalidate() and in pc_cmos_init().
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   block.c  |  101 ---------------------------------------------------
>   block.h  |   18 ---------
>   hw/fdc.c |  122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>   hw/fdc.h |   10 +++++-
>   hw/pc.c  |   13 ++-----
>   5 files changed, 124 insertions(+), 140 deletions(-)
>
> diff --git a/block.c b/block.c
> index f2540b9..fa1789c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2272,107 +2272,6 @@ void bdrv_set_io_limits(BlockDriverState *bs,
>       bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
>   }
>
> -/* Recognize floppy formats */
> -typedef struct FDFormat {
> -    FDriveType drive;
> -    uint8_t last_sect;
> -    uint8_t max_track;
> -    uint8_t max_head;
> -    FDriveRate rate;
> -} FDFormat;
> -
> -static const FDFormat fd_formats[] = {
> -    /* First entry is default format */
> -    /* 1.44 MB 3"1/2 floppy disks */
> -    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
> -    /* 2.88 MB 3"1/2 floppy disks */
> -    { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, },
> -    { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, },
> -    { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, },
> -    { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, },
> -    { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, },
> -    /* 720 kB 3"1/2 floppy disks */
> -    { FDRIVE_DRV_144,  9, 80, 1, FDRIVE_RATE_250K, },
> -    { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, },
> -    { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, },
> -    { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, },
> -    { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, },
> -    { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, },
> -    /* 1.2 MB 5"1/4 floppy disks */
> -    { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, },
> -    /* 720 kB 5"1/4 floppy disks */
> -    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
> -    { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, },
> -    /* 360 kB 5"1/4 floppy disks */
> -    { FDRIVE_DRV_120,  9, 40, 1, FDRIVE_RATE_300K, },
> -    { FDRIVE_DRV_120,  9, 40, 0, FDRIVE_RATE_300K, },
> -    { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, },
> -    { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, },
> -    /* 320 kB 5"1/4 floppy disks */
> -    { FDRIVE_DRV_120,  8, 40, 1, FDRIVE_RATE_250K, },
> -    { FDRIVE_DRV_120,  8, 40, 0, FDRIVE_RATE_250K, },
> -    /* 360 kB must match 5"1/4 better than 3"1/2... */
> -    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
> -    /* end */
> -    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
> -};
> -
> -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
> -                                   int *max_track, int *last_sect,
> -                                   FDriveType drive_in, FDriveType *drive,
> -                                   FDriveRate *rate)
> -{
> -    const FDFormat *parse;
> -    uint64_t nb_sectors, size;
> -    int i, first_match, match;
> -
> -    bdrv_get_geometry(bs,&nb_sectors);
> -    match = -1;
> -    first_match = -1;
> -    for (i = 0; ; i++) {
> -        parse =&fd_formats[i];
> -        if (parse->drive == FDRIVE_DRV_NONE) {
> -            break;
> -        }
> -        if (drive_in == parse->drive ||
> -            drive_in == FDRIVE_DRV_NONE) {
> -            size = (parse->max_head + 1) * parse->max_track *
> -                parse->last_sect;
> -            if (nb_sectors == size) {
> -                match = i;
> -                break;
> -            }
> -            if (first_match == -1) {
> -                first_match = i;
> -            }
> -        }
> -    }
> -    if (match == -1) {
> -        if (first_match == -1) {
> -            match = 1;
> -        } else {
> -            match = first_match;
> -        }
> -        parse =&fd_formats[match];
> -    }
> -    *nb_heads = parse->max_head + 1;
> -    *max_track = parse->max_track;
> -    *last_sect = parse->last_sect;
> -    *drive = parse->drive;
> -    *rate = parse->rate;
> -}
> -
>   int bdrv_get_translation_hint(BlockDriverState *bs)
>   {
>       return bs->translation;
> diff --git a/block.h b/block.h
> index 3af93c6..c1c7500 100644
> --- a/block.h
> +++ b/block.h
> @@ -267,24 +267,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs,
>   void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
>   void bdrv_get_geometry_hint(BlockDriverState *bs,
>                               int *pcyls, int *pheads, int *psecs);
> -typedef enum FDriveType {
> -    FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
> -    FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
> -    FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
> -    FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
> -} FDriveType;
> -
> -typedef enum FDriveRate {
> -    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> -    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> -    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> -    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> -} FDriveRate;
> -
> -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
> -                                   int *max_track, int *last_sect,
> -                                   FDriveType drive_in, FDriveType *drive,
> -                                   FDriveRate *rate);
>   int bdrv_get_translation_hint(BlockDriverState *bs);
>   void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
>                          BlockErrorAction on_write_error);
> diff --git a/hw/fdc.c b/hw/fdc.c
> index edf0706..41191c7 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -52,6 +52,113 @@
>   /********************************************************/
>   /* Floppy drive emulation                               */
>
> +typedef enum FDriveRate {
> +    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> +    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> +    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> +    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> +} FDriveRate;
> +
> +typedef struct FDFormat {
> +    FDriveType drive;
> +    uint8_t last_sect;
> +    uint8_t max_track;
> +    uint8_t max_head;
> +    FDriveRate rate;
> +} FDFormat;
> +
> +static const FDFormat fd_formats[] = {
> +    /* First entry is default format */
> +    /* 1.44 MB 3"1/2 floppy disks */
> +    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
> +    /* 2.88 MB 3"1/2 floppy disks */
> +    { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, },
> +    { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, },
> +    { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, },
> +    { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, },
> +    { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, },
> +    /* 720 kB 3"1/2 floppy disks */
> +    { FDRIVE_DRV_144,  9, 80, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, },
> +    /* 1.2 MB 5"1/4 floppy disks */
> +    { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, },
> +    /* 720 kB 5"1/4 floppy disks */
> +    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, },
> +    /* 360 kB 5"1/4 floppy disks */
> +    { FDRIVE_DRV_120,  9, 40, 1, FDRIVE_RATE_300K, },
> +    { FDRIVE_DRV_120,  9, 40, 0, FDRIVE_RATE_300K, },
> +    { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, },
> +    { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, },
> +    /* 320 kB 5"1/4 floppy disks */
> +    { FDRIVE_DRV_120,  8, 40, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_120,  8, 40, 0, FDRIVE_RATE_250K, },
> +    /* 360 kB must match 5"1/4 better than 3"1/2... */
> +    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
> +    /* end */
> +    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
> +};
> +
> +static void pick_geometry(BlockDriverState *bs, int *nb_heads,
> +                          int *max_track, int *last_sect,
> +                          FDriveType drive_in, FDriveType *drive,
> +                          FDriveRate *rate)
> +{
> +    const FDFormat *parse;
> +    uint64_t nb_sectors, size;
> +    int i, first_match, match;
> +
> +    bdrv_get_geometry(bs,&nb_sectors);
> +    match = -1;
> +    first_match = -1;
> +    for (i = 0; ; i++) {
> +        parse =&fd_formats[i];
> +        if (parse->drive == FDRIVE_DRV_NONE) {
> +            break;
> +        }
> +        if (drive_in == parse->drive ||
> +            drive_in == FDRIVE_DRV_NONE) {
> +            size = (parse->max_head + 1) * parse->max_track *
> +                parse->last_sect;
> +            if (nb_sectors == size) {
> +                match = i;
> +                break;
> +            }
> +            if (first_match == -1) {
> +                first_match = i;
> +            }
> +        }
> +    }
> +    if (match == -1) {
> +        if (first_match == -1) {
> +            match = 1;
> +        } else {
> +            match = first_match;
> +        }
> +        parse =&fd_formats[match];
> +    }
> +    *nb_heads = parse->max_head + 1;
> +    *max_track = parse->max_track;
> +    *last_sect = parse->last_sect;
> +    *drive = parse->drive;
> +    *rate = parse->rate;
> +}
> +
>   #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
>   #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
>
> @@ -187,8 +294,8 @@ static void fd_revalidate(FDrive *drv)
>       FLOPPY_DPRINTF("revalidate\n");
>       if (drv->bs != NULL) {
>           ro = bdrv_is_read_only(drv->bs);
> -        bdrv_get_floppy_geometry_hint(drv->bs,&nb_heads,&max_track,
> -&last_sect, drv->drive,&drive,&rate);
> +        pick_geometry(drv->bs,&nb_heads,&max_track,
> +&last_sect, drv->drive,&drive,&rate);
>           if (!bdrv_is_inserted(drv->bs)) {
>               FLOPPY_DPRINTF("No disk in drive\n");
>           } else {
> @@ -2054,18 +2161,13 @@ static int sun4m_fdc_init1(SysBusDevice *dev)
>       return fdctrl_init_common(fdctrl);
>   }
>
> -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev)
> +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
>   {
> -    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, dev);
> -    FDCtrl *fdctrl =&isa->state;
> -    int i;
> +    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, fdc);
>
> -    for (i = 0; i<  MAX_FD; i++) {
> -        bs[i] = fdctrl->drives[i].bs;
> -    }
> +    return isa->state.drives[i].drive;
>   }
>
> -
>   static const VMStateDescription vmstate_isa_fdc ={
>       .name = "fdc",
>       .version_id = 2,
> diff --git a/hw/fdc.h b/hw/fdc.h
> index 1b32b17..b5c9f31 100644
> --- a/hw/fdc.h
> +++ b/hw/fdc.h
> @@ -6,11 +6,19 @@
>   /* fdc.c */
>   #define MAX_FD 2
>
> +typedef enum FDriveType {
> +    FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
> +    FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
> +    FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
> +    FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
> +} FDriveType;
> +
>   ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
>   void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>                           target_phys_addr_t mmio_base, DriveInfo **fds);
>   void sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base,
>                          DriveInfo **fds, qemu_irq *fdc_tc);
> -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev);
> +
> +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
>
>   #endif
> diff --git a/hw/pc.c b/hw/pc.c
> index c7e9ab3..e5e7647 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>                     ISADevice *floppy, BusState *idebus0, BusState *idebus1,
>                     ISADevice *s)
>   {
> -    int val, nb, nb_heads, max_track, last_sect, i;
> -    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
> -    FDriveRate rate;
> -    BlockDriverState *fd[MAX_FD];
> +    int val, nb, i;
> +    FDriveType fd_type[2];

This results in:

   CC    i386-softmmu/hw/i386/../pc.o
/home/anthony/git/qemu/hw/i386/../pc.c: In function ‘pc_cmos_init’:
/home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[1]’ may be used 
uninitialized in this function [-Werror=uninitialized]
/home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[0]’ may be used 
uninitialized in this function [-Werror=uninitialized]
cc1: all warnings being treated as errors

And GCC is right as:

>       static pc_cmos_init_late_arg arg;
>
>       /* various important CMOS locations needed by PC/Bochs bios */
> @@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>
>       /* floppy type */
>       if (floppy) {
> -        fdc_get_bs(fd, floppy);
>           for (i = 0; i<  2; i++) {
> -            if (fd[i]) {
> -                bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
> -&last_sect, FDRIVE_DRV_NONE,
> -&fd_type[i],&rate);
> -            }
> +            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
>           }
>       }
>       val = (cmos_get_fd_drive_type(fd_type[0])<<  4) |

This is an unconditional use of fd_type[0].  If floppy == NULL, this is 
dereferencing an uninitialized value.

I'm not sure why the explicit initialization was removed...

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Kevin Wolf]

Am 09.07.2012 17:01, schrieb Anthony Liguori:
> On 07/09/2012 09:16 AM, Kevin Wolf wrote:
>> From: Markus Armbruster<armbru@redhat.com>
>>
>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>> functions already reside in block.c".  Device-specific functionality
>> should be kept in device code, not the block layer.  Move it back.
>>
>> Disk geometry guessing is still in block.c.  To be moved out in a
>> later patch series.
>>
>> Bonus: the floppy type used in pc_cmos_init() now obviously matches
>> the one in the FDrive.  Before, we relied on
>> bdrv_get_floppy_geometry_hint() picking the same type both in
>> fd_revalidate() and in pc_cmos_init().
>>
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

>> diff --git a/hw/pc.c b/hw/pc.c
>> index c7e9ab3..e5e7647 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>                     ISADevice *floppy, BusState *idebus0, BusState *idebus1,
>>                     ISADevice *s)
>>   {
>> -    int val, nb, nb_heads, max_track, last_sect, i;
>> -    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>> -    FDriveRate rate;
>> -    BlockDriverState *fd[MAX_FD];
>> +    int val, nb, i;
>> +    FDriveType fd_type[2];
> 
> This results in:
> 
>    CC    i386-softmmu/hw/i386/../pc.o
> /home/anthony/git/qemu/hw/i386/../pc.c: In function ‘pc_cmos_init’:
> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[1]’ may be used 
> uninitialized in this function [-Werror=uninitialized]
> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[0]’ may be used 
> uninitialized in this function [-Werror=uninitialized]
> cc1: all warnings being treated as errors
> 
> And GCC is right as:
> 
>>       static pc_cmos_init_late_arg arg;
>>
>>       /* various important CMOS locations needed by PC/Bochs bios */
>> @@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>
>>       /* floppy type */
>>       if (floppy) {
>> -        fdc_get_bs(fd, floppy);
>>           for (i = 0; i<  2; i++) {
>> -            if (fd[i]) {
>> -                bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
>> -&last_sect, FDRIVE_DRV_NONE,
>> -&fd_type[i],&rate);
>> -            }
>> +            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
>>           }
>>       }
>>       val = (cmos_get_fd_drive_type(fd_type[0])<<  4) |
> 
> This is an unconditional use of fd_type[0].  If floppy == NULL, this is 
> dereferencing an uninitialized value.
> 
> I'm not sure why the explicit initialization was removed...

Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
complain.

I dropped this patch from for-anthony, so you can give the pull request
another try.

Kevin

Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Anthony Liguori]

On 07/09/2012 10:24 AM, Kevin Wolf wrote:
> Am 09.07.2012 17:01, schrieb Anthony Liguori:
>> On 07/09/2012 09:16 AM, Kevin Wolf wrote:
>>> From: Markus Armbruster<armbru@redhat.com>
>>>
>>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>>> functions already reside in block.c".  Device-specific functionality
>>> should be kept in device code, not the block layer.  Move it back.
>>>
>>> Disk geometry guessing is still in block.c.  To be moved out in a
>>> later patch series.
>>>
>>> Bonus: the floppy type used in pc_cmos_init() now obviously matches
>>> the one in the FDrive.  Before, we relied on
>>> bdrv_get_floppy_geometry_hint() picking the same type both in
>>> fd_revalidate() and in pc_cmos_init().
>>>
>>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index c7e9ab3..e5e7647 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>>                      ISADevice *floppy, BusState *idebus0, BusState *idebus1,
>>>                      ISADevice *s)
>>>    {
>>> -    int val, nb, nb_heads, max_track, last_sect, i;
>>> -    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>>> -    FDriveRate rate;
>>> -    BlockDriverState *fd[MAX_FD];
>>> +    int val, nb, i;
>>> +    FDriveType fd_type[2];
>>
>> This results in:
>>
>>     CC    i386-softmmu/hw/i386/../pc.o
>> /home/anthony/git/qemu/hw/i386/../pc.c: In function ‘pc_cmos_init’:
>> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[1]’ may be used
>> uninitialized in this function [-Werror=uninitialized]
>> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[0]’ may be used
>> uninitialized in this function [-Werror=uninitialized]
>> cc1: all warnings being treated as errors
>>
>> And GCC is right as:
>>
>>>        static pc_cmos_init_late_arg arg;
>>>
>>>        /* various important CMOS locations needed by PC/Bochs bios */
>>> @@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>>
>>>        /* floppy type */
>>>        if (floppy) {
>>> -        fdc_get_bs(fd, floppy);
>>>            for (i = 0; i<   2; i++) {
>>> -            if (fd[i]) {
>>> -                bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
>>> -&last_sect, FDRIVE_DRV_NONE,
>>> -&fd_type[i],&rate);
>>> -            }
>>> +            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
>>>            }
>>>        }
>>>        val = (cmos_get_fd_drive_type(fd_type[0])<<   4) |
>>
>> This is an unconditional use of fd_type[0].  If floppy == NULL, this is
>> dereferencing an uninitialized value.
>>
>> I'm not sure why the explicit initialization was removed...
>
> Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
> complain.

:-)

> I dropped this patch from for-anthony, so you can give the pull request
> another try.

Okay, am building now and testing.  Looks good so far.

Regards,

Anthony Liguori

>
> Kevin

Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.07.2012 17:01, schrieb Anthony Liguori:
>> On 07/09/2012 09:16 AM, Kevin Wolf wrote:
>>> From: Markus Armbruster<armbru@redhat.com>
>>>
>>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>>> functions already reside in block.c".  Device-specific functionality
>>> should be kept in device code, not the block layer.  Move it back.
>>>
>>> Disk geometry guessing is still in block.c.  To be moved out in a
>>> later patch series.
>>>
>>> Bonus: the floppy type used in pc_cmos_init() now obviously matches
>>> the one in the FDrive.  Before, we relied on
>>> bdrv_get_floppy_geometry_hint() picking the same type both in
>>> fd_revalidate() and in pc_cmos_init().
>>>
>>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index c7e9ab3..e5e7647 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>>                     ISADevice *floppy, BusState *idebus0, BusState *idebus1,
>>>                     ISADevice *s)
>>>   {
>>> -    int val, nb, nb_heads, max_track, last_sect, i;
>>> -    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>>> -    FDriveRate rate;
>>> -    BlockDriverState *fd[MAX_FD];
>>> +    int val, nb, i;
>>> +    FDriveType fd_type[2];
>> 
>> This results in:
>> 
>>    CC    i386-softmmu/hw/i386/../pc.o
>> /home/anthony/git/qemu/hw/i386/../pc.c: In function ‘pc_cmos_init’:
>> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[1]’ may be used 
>> uninitialized in this function [-Werror=uninitialized]
>> /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[0]’ may be used 
>> uninitialized in this function [-Werror=uninitialized]
>> cc1: all warnings being treated as errors
>> 
>> And GCC is right as:
>> 
>>>       static pc_cmos_init_late_arg arg;
>>>
>>>       /* various important CMOS locations needed by PC/Bochs bios */
>>> @@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>>
>>>       /* floppy type */
>>>       if (floppy) {
>>> -        fdc_get_bs(fd, floppy);
>>>           for (i = 0; i<  2; i++) {
>>> -            if (fd[i]) {
>>> -                bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track,
>>> -&last_sect, FDRIVE_DRV_NONE,
>>> -&fd_type[i],&rate);
>>> -            }
>>> +            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
>>>           }
>>>       }
>>>       val = (cmos_get_fd_drive_type(fd_type[0])<<  4) |
>> 
>> This is an unconditional use of fd_type[0].  If floppy == NULL, this is 
>> dereferencing an uninitialized value.
>> 
>> I'm not sure why the explicit initialization was removed...

Brain fart on my part, sorry.  The old loop assigns only if the drive
exists.  The new loop assigns unconditionally.  Except the whole loop is
still conditional.

Testing can't flag this, because floppy is never null.

> Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
> complain.

Me too.  Looks like I should upgrade to a more recent gcc.

> I dropped this patch from for-anthony, so you can give the pull request
> another try.

I'll respin when the merge dust settled.

Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

On 07/09/2012 10:07 AM, Markus Armbruster wrote:

>>> This is an unconditional use of fd_type[0].  If floppy == NULL, this is 
>>> dereferencing an uninitialized value.
>>>
>>> I'm not sure why the explicit initialization was removed...
> 
> Brain fart on my part, sorry.  The old loop assigns only if the drive
> exists.  The new loop assigns unconditionally.  Except the whole loop is
> still conditional.
> 
> Testing can't flag this, because floppy is never null.
> 
>> Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
>> complain.
> 
> Me too.  Looks like I should upgrade to a more recent gcc.

It's probably not the version of the gcc you used, but whether or not
your CFLAGS include -O2.  Gcc has the (IMO very annoying) limitation
that uninitialized-use analysis can only be performed if you are also
doing optimization.  You have to use a tool like clang or Coverity if
you want more reliable uninitialized-use analysis even while building
-O0 debug images.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Anthony Liguori]

On 07/09/2012 09:16 AM, Kevin Wolf wrote:
> The following changes since commit 84988cf910a6881f2180fdcec516b60f8f0dc8c4:
>
>    bitops.h: Add functions to extract and deposit bitfields (2012-07-07 09:07:01 +0000)
>
> are available in the git repository at:
>
>    git://repo.or.cz/qemu/kevin.git for-anthony

Pulled updated versiong.  Thanks.

Regards,

Anthony Liguori

>
> MORITA Kazutaka (6):
>        sheepdog: fix dprintf format strings
>        sheepdog: restart I/O when socket becomes ready in do_co_req()
>        sheepdog: use coroutine based socket functions in coroutine context
>        sheepdog: make sure we don't free aiocb before sending all requests
>        sheepdog: split outstanding list into inflight and pending
>        sheepdog: traverse pending_list from the first for each time
>
> Markus Armbruster (4):
>        fdc: Drop broken code for user-defined floppy geometry
>        fdc: Move floppy geometry guessing back from block.c
>        qtest: Tidy up temporary files properly
>        block: Factor bdrv_read_unthrottled() out of guess_disk_lchs()
>
> Paolo Bonzini (8):
>        blkdebug: remove sync i/o events
>        blkdebug: tiny cleanup
>        blkdebug: pass getlength to underlying file
>        blkdebug: store list of active rules
>        blkdebug: optionally tie errors to a specific sector
>        raw: hook into blkdebug
>        block: copy over job and dirty bitmap fields in bdrv_append
>        block: introduce bdrv_swap, implement bdrv_append on top of it
>
> Pavel Hrdina (4):
>        fdc: rewrite seek and DSKCHG bit handling
>        fdc: fix interrupt handling
>        fdc_test: update media_change test
>        fdc_test: introduce test_sense_interrupt
>
> Stefan Hajnoczi (3):
>        qcow2: fix #ifdef'd qcow2_check_refcounts() callers
>        qcow2: preserve free_byte_offset when qcow2_alloc_bytes() fails
>        blockdev: warn when copy_on_read=on and readonly=on
>
>   block.c                |  306 +++++++++++++++++++-----------------------------
>   block.h                |   23 +---
>   block/blkdebug.c       |  107 ++++++++++-------
>   block/qcow2-refcount.c |    7 +-
>   block/qcow2-snapshot.c |    6 +-
>   block/qcow2.c          |    2 +-
>   block/qed.c            |    2 +-
>   block/raw.c            |    2 +
>   block/sheepdog.c       |  130 +++++++++++++--------
>   blockdev.c             |    4 +
>   hw/fdc.c               |  238 +++++++++++++++++++++++++++----------
>   hw/fdc.h               |   10 ++-
>   hw/pc.c                |   13 +--
>   tests/fdc-test.c       |   50 +++++++--
>   tests/libqtest.c       |   29 +++--
>   15 files changed, 522 insertions(+), 407 deletions(-)
>
>

Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Anthony Liguori]

On 07/09/2012 11:46 AM, Eric Blake wrote:
> On 07/09/2012 10:07 AM, Markus Armbruster wrote:
>
>>>> This is an unconditional use of fd_type[0].  If floppy == NULL, this is
>>>> dereferencing an uninitialized value.
>>>>
>>>> I'm not sure why the explicit initialization was removed...
>>
>> Brain fart on my part, sorry.  The old loop assigns only if the drive
>> exists.  The new loop assigns unconditionally.  Except the whole loop is
>> still conditional.
>>
>> Testing can't flag this, because floppy is never null.
>>
>>> Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
>>> complain.
>>
>> Me too.  Looks like I should upgrade to a more recent gcc.
>
> It's probably not the version of the gcc you used, but whether or not
> your CFLAGS include -O2.  Gcc has the (IMO very annoying) limitation
> that uninitialized-use analysis can only be performed if you are also
> doing optimization.  You have to use a tool like clang or Coverity if
> you want more reliable uninitialized-use analysis even while building
> -O0 debug images.
>

Specifically, without -O, GCC doesn't do data flow analysis so any warning that 
requires DFA won't get triggered.

So in general, if you are normally building with -O0, make sure to also build 
with -O in order to get full warnings.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c

[Kevin Wolf]

Am 09.07.2012 19:01, schrieb Anthony Liguori:
> On 07/09/2012 11:46 AM, Eric Blake wrote:
>> On 07/09/2012 10:07 AM, Markus Armbruster wrote:
>>
>>>>> This is an unconditional use of fd_type[0].  If floppy == NULL, this is
>>>>> dereferencing an uninitialized value.
>>>>>
>>>>> I'm not sure why the explicit initialization was removed...
>>>
>>> Brain fart on my part, sorry.  The old loop assigns only if the drive
>>> exists.  The new loop assigns unconditionally.  Except the whole loop is
>>> still conditional.
>>>
>>> Testing can't flag this, because floppy is never null.
>>>
>>>> Looks broken indeed. I just wonder why my gcc (or the buildbots) didn't
>>>> complain.
>>>
>>> Me too.  Looks like I should upgrade to a more recent gcc.
>>
>> It's probably not the version of the gcc you used, but whether or not
>> your CFLAGS include -O2.  Gcc has the (IMO very annoying) limitation
>> that uninitialized-use analysis can only be performed if you are also
>> doing optimization.  You have to use a tool like clang or Coverity if
>> you want more reliable uninitialized-use analysis even while building
>> -O0 debug images.
>>
> 
> Specifically, without -O, GCC doesn't do data flow analysis so any warning that 
> requires DFA won't get triggered.
> 
> So in general, if you are normally building with -O0, make sure to also build 
> with -O in order to get full warnings.

Just checked it to be sure, this doesn't seem to be the reason:

CFLAGS=-O2 -D_FORTIFY_SOURCE=2 -g

Kevin