Block patches

GIT PULL In review Kevin Wolf 3 participants 26 messages Jun 15, 2010 Block layer

lore

Share & tools

lore

Kevin Wolf

author

2010-06-15 14:20 UTC #0 lore raw

The following changes since commit fd42deeb4cb42f90084046e3ebdb4383953195e3:
Gerd Hoffmann (1):
Add exit notifiers.

are available in the git repository at:

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

Blue Swirl (1):
block: fix a warning and possible truncation

Christoph Hellwig (3):
cow: use pread/pwrite
cow: stop using mmap
cow: use qemu block API

Jan Kiszka (1):
xen: Fix build error due to missing include

Jes Sorensen (1):
Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE

Kevin Wolf (5):
vpc: Read/write multiple sectors at once
qcow2: Allow get_refcount to return errors
qcow2: Allow alloc_clusters_noref to return errors
qcow2: Return real error code in load_refcount_block
qcow2: Restore L1 entry on l2_allocate failure

Markus Armbruster (7):
Fix regression for "-drive file="
block: Move error actions from DriveInfo to BlockDriverState
block: Decouple block device "commit all" from DriveInfo
monitor: Make "commit FOO" complain when FOO doesn't exist
block: New bdrv_next()
block: Decouple savevm from DriveInfo
blockdev: Give drives internal linkage

Miguel Di Ciurcio Filho (1):
savevm: Really verify if a drive supports snapshots

 block.c                |   49 ++++++++++++++++++-
 block.h                |   11 ++++
 block/cow.c            |  127 ++++++++++++++++++++++++++----------------------
 block/qcow2-cluster.c  |    1 +
 block/qcow2-refcount.c |   70 +++++++++++++++++++++++----
 block/vpc.c            |   41 +++++++++++----
 block_int.h            |    1 +
 blockdev.c             |   39 +++++---------
 blockdev.h             |   12 -----
 hw/fdc.c               |    4 +-
 hw/ide/core.c          |    2 +-
 hw/scsi-disk.c         |    2 +-
 hw/virtio-blk.c        |    3 +-
 hw/xen_backend.h       |    1 +
 qemu-char.c            |    7 +--
 qemu-common.h          |    3 -
 qemu-malloc.c          |    5 --
 savevm.c               |   90 +++++++++++++++++++---------------
 18 files changed, 291 insertions(+), 177 deletions(-)

Kevin Wolf

author

2010-06-15 14:20 UTC #1 lore raw

PATCH 1/19 vpc: Read/write multiple sectors at once

This changes the vpc block driver (for VHD) to read/write multiple sectors at once instead of doing a request for each single sector.

Before this, running qemu-iotests for VPC took ages, now it's actually quite reasonable to run it always (down from ~1 hour to 40 seconds for me).

Signed-off-by Kevin Wolf <[email protected]>

---
 block/vpc.c |   41 ++++++++++++++++++++++++++++++-----------
 1 files changed, 30 insertions(+), 11 deletions(-)

block/vpc.c +30 ~~-11~~

index 214e9d1..f1f73e2 100644
@@ -371,23 +371,33 @@ fail:
 static int vpc_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
+    BDRVVPCState *s = bs->opaque;
     int ret;
     int64_t offset;
+    int64_t sectors, sectors_per_block;
 
     while (nb_sectors > 0) {
         offset = get_sector_offset(bs, sector_num, 0);
 
+        sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
+        sectors = sectors_per_block - (sector_num % sectors_per_block);
+        if (sectors > nb_sectors) {
+            sectors = nb_sectors;
+        }
+
         if (offset == -1) {
-            memset(buf, 0, 512);
+            memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
         } else {
-            ret = bdrv_pread(bs->file, offset, buf, 512);
-            if (ret != 512)
+            ret = bdrv_pread(bs->file, offset, buf,
+                sectors * BDRV_SECTOR_SIZE);
+            if (ret != sectors * BDRV_SECTOR_SIZE) {
                 return -1;
+            }
         }
 
-        nb_sectors--;
-        sector_num++;
-        buf += 512;
+        nb_sectors -= sectors;
+        sector_num += sectors;
+        buf += sectors * BDRV_SECTOR_SIZE;
     }
     return 0;
 }

@@ -395,25 +405,34 @@ static int vpc_read(BlockDriverState *bs, int64_t sector_num,
 static int vpc_write(BlockDriverState *bs, int64_t sector_num,
     const uint8_t *buf, int nb_sectors)
 {
+    BDRVVPCState *s = bs->opaque;
     int64_t offset;
+    int64_t sectors, sectors_per_block;
     int ret;
 
     while (nb_sectors > 0) {
         offset = get_sector_offset(bs, sector_num, 1);
 
+        sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
+        sectors = sectors_per_block - (sector_num % sectors_per_block);
+        if (sectors > nb_sectors) {
+            sectors = nb_sectors;
+        }
+
         if (offset == -1) {
             offset = alloc_block(bs, sector_num);
             if (offset < 0)
                 return -1;
         }
 
-        ret = bdrv_pwrite(bs->file, offset, buf, 512);
-        if (ret != 512)
+        ret = bdrv_pwrite(bs->file, offset, buf, sectors * BDRV_SECTOR_SIZE);
+        if (ret != sectors * BDRV_SECTOR_SIZE) {
             return -1;
+        }
 
-        nb_sectors--;
-        sector_num++;
-        buf += 512;
+        nb_sectors -= sectors;
+        sector_num += sectors;
+        buf += sectors * BDRV_SECTOR_SIZE;
     }
 
     return 0;

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #2 lore raw

PATCH 2/19 qcow2: Allow get_refcount to return errors

In reply to Kevin Wolf

get_refcount might need to load a refcount block from disk, so errors may happen. Return the error code instead of assuming a refcount of 1 and change the callers to respect error return values.

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2-refcount.c |   41 +++++++++++++++++++++++++++++++++++++----
 1 files changed, 37 insertions(+), 4 deletions(-)

block/qcow2-refcount.c +37 -4

index 22b0b45..ca6b373 100644
@@ -105,11 +105,17 @@ static int load_refcount_block(BlockDriverState *bs,
     return 0;
 }
 
+/*
+ * Returns the refcount of the cluster given by its index. Any non-negative
+ * return value is the refcount of the cluster, negative values are -errno
+ * and indicate an error.
+ */
 static int get_refcount(BlockDriverState *bs, int64_t cluster_index)
 {
     BDRVQcowState *s = bs->opaque;
     int refcount_table_index, block_index;
     int64_t refcount_block_offset;
+    int ret;
 
     refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
     if (refcount_table_index >= s->refcount_table_size)

@@ -119,8 +125,10 @@ static int get_refcount(BlockDriverState *bs, int64_t cluster_index)
         return 0;
     if (refcount_block_offset != s->refcount_block_cache_offset) {
         /* better than nothing: return allocated if read error */
-        if (load_refcount_block(bs, refcount_block_offset) < 0)
-            return 1;
+        ret = load_refcount_block(bs, refcount_block_offset);
+        if (ret < 0) {
+            return ret;
+        }
     }
     block_index = cluster_index &
         ((1 << (s->cluster_bits - REFCOUNT_SHIFT)) - 1);

@@ -538,7 +546,13 @@ fail:
     return ret;
 }
 
-/* addend must be 1 or -1 */
+/*
+ * Increases or decreases the refcount of a given cluster by one.
+ * addend must be 1 or -1.
+ *
+ * If the return value is non-negative, it is the new refcount of the cluster.
+ * If it is negative, it is -errno and indicates an error.
+ */
 static int update_cluster_refcount(BlockDriverState *bs,
                                    int64_t cluster_index,
                                    int addend)

@@ -779,6 +793,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                         } else {
                             refcount = get_refcount(bs, offset >> s->cluster_bits);
                         }
+
+                        if (refcount < 0) {
+                            goto fail;
+                        }
                     }
 
                     if (refcount == 1) {

@@ -801,7 +819,9 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
             } else {
                 refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
             }
-            if (refcount == 1) {
+            if (refcount < 0) {
+                goto fail;
+            } else if (refcount == 1) {
                 l2_offset |= QCOW_OFLAG_COPIED;
             }
             if (l2_offset != old_l2_offset) {

@@ -934,6 +954,10 @@ static int check_refcounts_l2(BlockDriverState *bs,
                     uint64_t entry = offset;
                     offset &= ~QCOW_OFLAG_COPIED;
                     refcount = get_refcount(bs, offset >> s->cluster_bits);
+                    if (refcount < 0) {
+                        fprintf(stderr, "Can't get refcount for offset %"
+                            PRIx64 ": %s\n", entry, strerror(-refcount));
+                    }
                     if ((refcount == 1) != ((entry & QCOW_OFLAG_COPIED) != 0)) {
                         fprintf(stderr, "ERROR OFLAG_COPIED: offset=%"
                             PRIx64 " refcount=%d\n", entry, refcount);

@@ -1011,6 +1035,10 @@ static int check_refcounts_l1(BlockDriverState *bs,
             if (check_copied) {
                 refcount = get_refcount(bs, (l2_offset & ~QCOW_OFLAG_COPIED)
                     >> s->cluster_bits);
+                if (refcount < 0) {
+                    fprintf(stderr, "Can't get refcount for l2_offset %"
+                        PRIx64 ": %s\n", l2_offset, strerror(-refcount));
+                }
                 if ((refcount == 1) != ((l2_offset & QCOW_OFLAG_COPIED) != 0)) {
                     fprintf(stderr, "ERROR OFLAG_COPIED: l2_offset=%" PRIx64
                         " refcount=%d\n", l2_offset, refcount);

@@ -1118,6 +1146,11 @@ int qcow2_check_refcounts(BlockDriverState *bs)
     /* compare ref counts */
     for(i = 0; i < nb_clusters; i++) {
         refcount1 = get_refcount(bs, i);
+        if (refcount1 < 0) {
+            fprintf(stderr, "Can't get refcount for cluster %d: %s\n",
+                i, strerror(-refcount1));
+        }
+
         refcount2 = refcount_table[i];
         if (refcount1 != refcount2) {
             fprintf(stderr, "ERROR cluster %d refcount=%d reference=%d\n",

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #3 lore raw

PATCH 3/19 qcow2: Allow alloc_clusters_noref to return errors

In reply to Kevin Wolf

Currently it would consider blocks for which get_refcount fails used. However, it's unlikely that get_refcount would succeed for the next cluster, so it's not really helpful. Return an error instead.

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2-refcount.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

block/qcow2-refcount.c +15 -3

index ca6b373..51948ae 100644
@@ -228,7 +228,10 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)
     }
 
     /* Allocate the refcount block itself and mark it as used */
-    uint64_t new_block = alloc_clusters_noref(bs, s->cluster_size);
+    int64_t new_block = alloc_clusters_noref(bs, s->cluster_size);
+    if (new_block < 0) {
+        return new_block;
+    }
 
 #ifdef DEBUG_ALLOC2
     fprintf(stderr, "qcow2: Allocate refcount block %d for %" PRIx64

@@ -579,14 +582,19 @@ static int update_cluster_refcount(BlockDriverState *bs,
 static int64_t alloc_clusters_noref(BlockDriverState *bs, int64_t size)
 {
     BDRVQcowState *s = bs->opaque;
-    int i, nb_clusters;
+    int i, nb_clusters, refcount;
 
     nb_clusters = size_to_clusters(s, size);
 retry:
     for(i = 0; i < nb_clusters; i++) {
         int64_t next_cluster_index = s->free_cluster_index++;
-        if (get_refcount(bs, next_cluster_index) != 0)
+        refcount = get_refcount(bs, next_cluster_index);
+
+        if (refcount < 0) {
+            return refcount;
+        } else if (refcount != 0) {
             goto retry;
+        }
     }
 #ifdef DEBUG_ALLOC2
     printf("alloc_clusters: size=%" PRId64 " -> %" PRId64 "\n",

@@ -603,6 +611,10 @@ int64_t qcow2_alloc_clusters(BlockDriverState *bs, int64_t size)
 
     BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC);
     offset = alloc_clusters_noref(bs, size);
+    if (offset < 0) {
+        return offset;
+    }
+
     ret = update_refcount(bs, offset, size, 1);
     if (ret < 0) {
         return ret;

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #4 lore raw

PATCH 4/19 qcow2: Return real error code in load_refcount_block

In reply to Kevin Wolf

This fixes load_refcount_block which completely ignored the return value of write_refcount_block and always returned -EIO for bdrv_pwrite failure.

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2-refcount.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

block/qcow2-refcount.c +8 -3

index 51948ae..41e1da9 100644
@@ -93,14 +93,19 @@ static int load_refcount_block(BlockDriverState *bs,
     int ret;
 
     if (cache_refcount_updates) {
-        write_refcount_block(bs);
+        ret = write_refcount_block(bs);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_LOAD);
     ret = bdrv_pread(bs->file, refcount_block_offset, s->refcount_block_cache,
                      s->cluster_size);
-    if (ret != s->cluster_size)
-        return -EIO;
+    if (ret < 0) {
+        return ret;
+    }
+
     s->refcount_block_cache_offset = refcount_block_offset;
     return 0;
 }

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #5 lore raw

PATCH 5/19 savevm: Really verify if a drive supports snapshots

In reply to Kevin Wolf

From: Miguel Di Ciurcio Filho <[email protected]>

Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.

First issue: Their names implies different porpouses, but they do the same thing and have exactly the same code. Maybe copied and pasted and forgotten? bdrv_has_snapshot() is called in various places for actually checking if there is snapshots or not.

Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or not snapshots does not catch all cases. E.g.: a raw image.

So when do_savevm() is called, first thing it does is to set a global BlockDriverState to save the VM memory state calling get_bs_snapshots().

static BlockDriverState *get_bs_snapshots(void)

{
    BlockDriverState *bs;
    DriveInfo *dinfo;

    if (bs_snapshots)
        return bs_snapshots;
    QTAILQ_FOREACH(dinfo, &drives, next) {
        bs = dinfo->bdrv;
        if (bdrv_can_snapshot(bs))
            goto ok;
    }
    return NULL;
 ok:
    bs_snapshots = bs;
    return bs;
}

bdrv_can_snapshot() may return a BlockDriverState that does not support snapshots and do_savevm() goes on.

Later on in do_savevm(), we find:

    QTAILQ_FOREACH(dinfo, &drives, next) {
        bs1 = dinfo->bdrv;
        if (bdrv_has_snapshot(bs1)) {
            /* Write VM state size only to the image that contains the state */
            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
            ret = bdrv_snapshot_create(bs1, sn);
            if (ret < 0) {
                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                               bdrv_get_device_name(bs1));
            }
        }
    }

bdrv_has_snapshot(bs1) is not checking if the device does support or has snapshots as explained above. Only in bdrv_snapshot_create() the device is actually checked for snapshot support.

So, in cases where the first device supports snapshots, and the second does not, the snapshot on the first will happen anyways. I believe this is not a good behavior. It should be an all or nothing process.

This patch addresses these issues by making bdrv_can_snapshot() actually do what it must do and enforces better tests to avoid errors in the middle of do_savevm(). bdrv_has_snapshot() is removed and replaced by bdrv_can_snapshot() where appropriate.

bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more sense to me.

The loadvm_state() function was updated too to enforce that when loading a VM at least all writable devices must support snapshots too.

Signed-off-by Miguel Di Ciurcio Filho <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c  |   17 +++++++++++++++++
 block.h  |    1 +
 savevm.c |   58 ++++++++++++++++++++++++++++++++++++----------------------
 3 files changed, 54 insertions(+), 22 deletions(-)

block.c +17 -0

index cacf11b..29a6f39 100644
@@ -1666,6 +1666,23 @@ void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
 /**************************************************************/
 /* handling of snapshots */
 
+int bdrv_can_snapshot(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv || bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
+        return 0;
+    }
+
+    if (!drv->bdrv_snapshot_create) {
+        if (bs->file != NULL) {
+            return bdrv_can_snapshot(bs->file);
+        }
+        return 0;
+    }
+
+    return 1;
+}
+
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info)
 {

block.h +1 -0

index 25744b1..a340ffd 100644
@@ -175,6 +175,7 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
+int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,

savevm.c +36 ~~-22~~

112 lines not on this page · raw message

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #6 lore raw

PATCH 6/19 Fix regression for "-drive file="

In reply to Kevin Wolf

From: Markus Armbruster <[email protected]>

Empty file used to create an empty drive (no media). Since commit 9dfd7c7a, it's an error: "qemu: could not open disk image : No such file or directory". Older versions of libvirt can choke on this.

Signed-off-by Markus Armbruster <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 blockdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

blockdev.c +1 -1

index 642ce75..dbeef09 100644
@@ -462,7 +462,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     case IF_COUNT:
         abort();
     }
-    if (!file) {
+    if (!file || !*file) {
         *fatal_error = 0;
         return NULL;
     }

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #7 lore raw

PATCH 7/19 qcow2: Restore L1 entry on l2_allocate failure

In reply to Kevin Wolf

If writing the L1 table to disk failed, we need to restore its old content in memory to avoid inconsistencies.

Reported-by Juan Quintela <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block/qcow2-cluster.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

block/qcow2-cluster.c +1 -0

index 03a9f25..5760ad6 100644
@@ -285,6 +285,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
     return 0;
 
 fail:
+    s->l1_table[l1_index] = old_l2_offset;
     qcow2_l2_cache_reset(bs);
     return ret;
 }

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #8 lore raw

PATCH 8/19 cow: use pread/pwrite

In reply to Kevin Wolf

From: Christoph Hellwig <[email protected]>

Use pread/pwrite instead of lseek + read/write in preparation of using the qemu block API.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block/cow.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

block/cow.c +5 -5

index fde066e..1a48ac0 100644
@@ -78,7 +78,7 @@ static int cow_open(BlockDriverState *bs, const char *filename, int flags)
     }
     s->fd = fd;
     /* see if it is a cow image */
-    if (read(fd, &cow_header, sizeof(cow_header)) != sizeof(cow_header)) {
+    if (pread(fd, &cow_header, sizeof(cow_header), 0) != sizeof(cow_header)) {
         goto fail;
     }

@@ -159,8 +159,8 @@ static int cow_read(BlockDriverState *bs, int64_t sector_num,
 
     while (nb_sectors > 0) {
         if (is_changed(s->cow_bitmap, sector_num, nb_sectors, &n)) {
-            lseek(s->fd, s->cow_sectors_offset + sector_num * 512, SEEK_SET);
-            ret = read(s->fd, buf, n * 512);
+            ret = pread(s->fd, buf, n * 512,
+                        s->cow_sectors_offset + sector_num * 512);
             if (ret != n * 512)
                 return -1;
         } else {

@@ -186,8 +186,8 @@ static int cow_write(BlockDriverState *bs, int64_t sector_num,
     BDRVCowState *s = bs->opaque;
     int ret, i;
 
-    lseek(s->fd, s->cow_sectors_offset + sector_num * 512, SEEK_SET);
-    ret = write(s->fd, buf, nb_sectors * 512);
+    ret = pwrite(s->fd, buf, nb_sectors * 512,
+                 s->cow_sectors_offset + sector_num * 512);
     if (ret != nb_sectors * 512)
         return -1;
     for (i = 0; i < nb_sectors; i++)

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #9 lore raw

PATCH 9/19 cow: stop using mmap

In reply to Kevin Wolf

From: Christoph Hellwig <[email protected]>

We don't have an equivalent to mmap in the qemu block API, so read and write the bitmap directly. At least in the dumb implementation added in this patch this is a lot less efficient, but it means cow can also work on windows, and over nbd or curl. And it fixes qemu-iotests testcase 012 which did not work properly due to issues with read-only mmap access.

In addition we can also get rid of the now unused get_mmap_addr function.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block/cow.c   |   98 +++++++++++++++++++++++++++++++++++---------------------
 qemu-common.h |    3 --
 qemu-malloc.c |    5 ---
 3 files changed, 61 insertions(+), 45 deletions(-)

block/cow.c +61 ~~-37~~

index 1a48ac0..ec9a9f7 100644
@@ -21,11 +21,9 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-#ifndef _WIN32
 #include "qemu-common.h"
 #include "block_int.h"
 #include "module.h"
-#include <sys/mman.h>
 
 /**************************************************************/
 /* COW block driver using file system holes */

@@ -45,9 +43,6 @@ struct cow_header_v2 {
 
 typedef struct BDRVCowState {
     int fd;
-    uint8_t *cow_bitmap; /* if non NULL, COW mappings are used first */
-    uint8_t *cow_bitmap_addr; /* mmap address of cow_bitmap */
-    int cow_bitmap_size;
     int64_t cow_sectors_offset;
 } BDRVCowState;

@@ -68,6 +63,7 @@ static int cow_open(BlockDriverState *bs, const char *filename, int flags)
     BDRVCowState *s = bs->opaque;
     int fd;
     struct cow_header_v2 cow_header;
+    int bitmap_size;
     int64_t size;
 
     fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);

@@ -94,61 +90,92 @@ static int cow_open(BlockDriverState *bs, const char *filename, int flags)
     pstrcpy(bs->backing_file, sizeof(bs->backing_file),
             cow_header.backing_file);
 
-    /* mmap the bitmap */
-    s->cow_bitmap_size = ((bs->total_sectors + 7) >> 3) + sizeof(cow_header);
-    s->cow_bitmap_addr = (void *)mmap(get_mmap_addr(s->cow_bitmap_size),
-                                      s->cow_bitmap_size,
-                                      PROT_READ | PROT_WRITE,
-                                      MAP_SHARED, s->fd, 0);
-    if (s->cow_bitmap_addr == MAP_FAILED)
-        goto fail;
-    s->cow_bitmap = s->cow_bitmap_addr + sizeof(cow_header);
-    s->cow_sectors_offset = (s->cow_bitmap_size + 511) & ~511;
+    bitmap_size = ((bs->total_sectors + 7) >> 3) + sizeof(cow_header);
+    s->cow_sectors_offset = (bitmap_size + 511) & ~511;
     return 0;
  fail:
     close(fd);
     return -1;
 }
 
-static inline void cow_set_bit(uint8_t *bitmap, int64_t bitnum)
+/*
+ * XXX(hch): right now these functions are extremly ineffcient.
+ * We should just read the whole bitmap we'll need in one go instead.
+ */
+static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
 {
-    bitmap[bitnum / 8] |= (1 << (bitnum%8));
+    BDRVCowState *s = bs->opaque;
+    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
+    uint8_t bitmap;
+
+    if (pread(s->fd, &bitmap, sizeof(bitmap), offset) !=
+	    sizeof(bitmap)) {
+       return -errno;
+    }
+
+    bitmap |= (1 << (bitnum % 8));
+
+    if (pwrite(s->fd, &bitmap, sizeof(bitmap), offset) !=
+	    sizeof(bitmap)) {
+       return -errno;
+    }
+    return 0;
 }
 
-static inline int is_bit_set(const uint8_t *bitmap, int64_t bitnum)
+static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
 {
-    return !!(bitmap[bitnum / 8] & (1 << (bitnum%8)));
-}
+    BDRVCowState *s = bs->opaque;
+    uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
+    uint8_t bitmap;
 
+    if (pread(s->fd, &bitmap, sizeof(bitmap), offset) !=
+	    sizeof(bitmap)) {
+       return -errno;
+    }
+
+    return !!(bitmap & (1 << (bitnum % 8)));
+}
 
 /* Return true if first block has been changed (ie. current version is
  * in COW file).  Set the number of continuous blocks for which that
  * is true. */
-static inline int is_changed(uint8_t *bitmap,
-                             int64_t sector_num, int nb_sectors,
-                             int *num_same)
+static int cow_is_allocated(BlockDriverState *bs, int64_t sector_num,
+        int nb_sectors, int *num_same)
 {
     int changed;
 
-    if (!bitmap || nb_sectors == 0) {
+    if (nb_sectors == 0) {
 	*num_same = nb_sectors;
 	return 0;
     }
 
-    changed = is_bit_set(bitmap, sector_num);
+    changed = is_bit_set(bs, sector_num);
+    if (changed < 0) {
+        return 0; /* XXX: how to return I/O errors? */
+    }
+
     for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
-	if (is_bit_set(bitmap, sector_num + *num_same) != changed)
+	if (is_bit_set(bs, sector_num + *num_same) != changed)
 	    break;
     }
 
     return changed;
 }
 
-static int cow_is_allocated(BlockDriverState *bs, int64_t sector_num,
-                            int nb_sectors, int *pnum)
+static int cow_update_bitmap(BlockDriverState *bs, int64_t sector_num,
+        int nb_sectors)
 {
-    BDRVCowState *s = bs->opaque;
-    return is_changed(s->cow_bitmap, sector_num, nb_sectors, pnum);
+    int error = 0;
+    int i;
+
+    for (i = 0; i < nb_sectors; i++) {
+        error = cow_set_bit(bs, sector_num + i);
+        if (error) {
+            break;
+        }
+    }
+
+    return error;
 }
 
 static int cow_read(BlockDriverState *bs, int64_t sector_num,

@@ -158,7 +185,7 @@ static int cow_read(BlockDriverState *bs, int64_t sector_num,
     int ret, n;
 
     while (nb_sectors > 0) {
-        if (is_changed(s->cow_bitmap, sector_num, nb_sectors, &n)) {
+        if (cow_is_allocated(bs, sector_num, nb_sectors, &n)) {
             ret = pread(s->fd, buf, n * 512,
                         s->cow_sectors_offset + sector_num * 512);
             if (ret != n * 512)

@@ -184,21 +211,19 @@ static int cow_write(BlockDriverState *bs, int64_t sector_num,
                      const uint8_t *buf, int nb_sectors)
 {
     BDRVCowState *s = bs->opaque;
-    int ret, i;
+    int ret;
 
     ret = pwrite(s->fd, buf, nb_sectors * 512,
                  s->cow_sectors_offset + sector_num * 512);
     if (ret != nb_sectors * 512)
         return -1;
-    for (i = 0; i < nb_sectors; i++)
-        cow_set_bit(s->cow_bitmap, sector_num + i);
-    return 0;
+
+    return cow_update_bitmap(bs, sector_num, nb_sectors);
 }
 
 static void cow_close(BlockDriverState *bs)
 {
     BDRVCowState *s = bs->opaque;
-    munmap((void *)s->cow_bitmap_addr, s->cow_bitmap_size);
     close(s->fd);
 }

@@ -308,4 +333,3 @@ static void bdrv_cow_init(void)
 }
 
 block_init(bdrv_cow_init);
-#endif

qemu-common.h +0 -3

index d133f35..00998c3 100644
@@ -164,9 +164,6 @@ void qemu_free(void *ptr);
 char *qemu_strdup(const char *str);
 char *qemu_strndup(const char *str, size_t size);
 
-void *get_mmap_addr(unsigned long size);
-
-
 void qemu_mutex_lock_iothread(void);
 void qemu_mutex_unlock_iothread(void);

qemu-malloc.c +0 -5

index 1b33e04..36b0b36 100644
@@ -32,11 +32,6 @@ static void *oom_check(void *ptr)
     return ptr;
 }
 
-void *get_mmap_addr(unsigned long size)
-{
-    return NULL;
-}
-
 void qemu_free(void *ptr)
 {
     free(ptr);

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #10 lore raw

PATCH 10/19 cow: use qemu block API

In reply to Kevin Wolf

From: Christoph Hellwig <[email protected]>

Use bdrv_pwrite to access the backing device instead of pread, and convert the driver to implementing the bdrv_open method which gives it an already opened BlockDriverState for the underlying device.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block/cow.c |   39 +++++++++++++--------------------------
 1 files changed, 13 insertions(+), 26 deletions(-)

block/cow.c +13 ~~-26~~

index ec9a9f7..d146434 100644
@@ -42,7 +42,6 @@ struct cow_header_v2 {
 };
 
 typedef struct BDRVCowState {
-    int fd;
     int64_t cow_sectors_offset;
 } BDRVCowState;

@@ -58,23 +57,16 @@ static int cow_probe(const uint8_t *buf, int buf_size, const char *filename)
         return 0;
 }
 
-static int cow_open(BlockDriverState *bs, const char *filename, int flags)
+static int cow_open(BlockDriverState *bs, int flags)
 {
     BDRVCowState *s = bs->opaque;
-    int fd;
     struct cow_header_v2 cow_header;
     int bitmap_size;
     int64_t size;
 
-    fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
-    if (fd < 0) {
-        fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
-        if (fd < 0)
-            return -1;
-    }
-    s->fd = fd;
     /* see if it is a cow image */
-    if (pread(fd, &cow_header, sizeof(cow_header), 0) != sizeof(cow_header)) {
+    if (bdrv_pread(bs->file, 0, &cow_header, sizeof(cow_header)) !=
+            sizeof(cow_header)) {
         goto fail;
     }

@@ -94,7 +86,6 @@ static int cow_open(BlockDriverState *bs, const char *filename, int flags)
     s->cow_sectors_offset = (bitmap_size + 511) & ~511;
     return 0;
  fail:
-    close(fd);
     return -1;
 }

@@ -104,18 +95,17 @@ static int cow_open(BlockDriverState *bs, const char *filename, int flags)
  */
 static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
 {
-    BDRVCowState *s = bs->opaque;
     uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
     uint8_t bitmap;
 
-    if (pread(s->fd, &bitmap, sizeof(bitmap), offset) !=
+    if (bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap)) !=
 	    sizeof(bitmap)) {
        return -errno;
     }
 
     bitmap |= (1 << (bitnum % 8));
 
-    if (pwrite(s->fd, &bitmap, sizeof(bitmap), offset) !=
+    if (bdrv_pwrite(bs->file, offset, &bitmap, sizeof(bitmap)) !=
 	    sizeof(bitmap)) {
        return -errno;
     }

@@ -124,11 +114,10 @@ static inline int cow_set_bit(BlockDriverState *bs, int64_t bitnum)
 
 static inline int is_bit_set(BlockDriverState *bs, int64_t bitnum)
 {
-    BDRVCowState *s = bs->opaque;
     uint64_t offset = sizeof(struct cow_header_v2) + bitnum / 8;
     uint8_t bitmap;
 
-    if (pread(s->fd, &bitmap, sizeof(bitmap), offset) !=
+    if (bdrv_pread(bs->file, offset, &bitmap, sizeof(bitmap)) !=
 	    sizeof(bitmap)) {
        return -errno;
     }

@@ -186,8 +175,9 @@ static int cow_read(BlockDriverState *bs, int64_t sector_num,
 
     while (nb_sectors > 0) {
         if (cow_is_allocated(bs, sector_num, nb_sectors, &n)) {
-            ret = pread(s->fd, buf, n * 512,
-                        s->cow_sectors_offset + sector_num * 512);
+            ret = bdrv_pread(bs->file,
+                        s->cow_sectors_offset + sector_num * 512,
+                        buf, n * 512);
             if (ret != n * 512)
                 return -1;
         } else {

@@ -213,8 +203,8 @@ static int cow_write(BlockDriverState *bs, int64_t sector_num,
     BDRVCowState *s = bs->opaque;
     int ret;
 
-    ret = pwrite(s->fd, buf, nb_sectors * 512,
-                 s->cow_sectors_offset + sector_num * 512);
+    ret = bdrv_pwrite(bs->file, s->cow_sectors_offset + sector_num * 512,
+                      buf, nb_sectors * 512);
     if (ret != nb_sectors * 512)
         return -1;

@@ -223,8 +213,6 @@ static int cow_write(BlockDriverState *bs, int64_t sector_num,
 
 static void cow_close(BlockDriverState *bs)
 {
-    BDRVCowState *s = bs->opaque;
-    close(s->fd);
 }
 
 static int cow_create(const char *filename, QEMUOptionParameter *options)

@@ -294,8 +282,7 @@ exit:
 
 static void cow_flush(BlockDriverState *bs)
 {
-    BDRVCowState *s = bs->opaque;
-    qemu_fdatasync(s->fd);
+    bdrv_flush(bs->file);
 }
 
 static QEMUOptionParameter cow_create_options[] = {

@@ -316,7 +303,7 @@ static BlockDriver bdrv_cow = {
     .format_name	= "cow",
     .instance_size	= sizeof(BDRVCowState),
     .bdrv_probe		= cow_probe,
-    .bdrv_file_open	= cow_open,
+    .bdrv_open		= cow_open,
     .bdrv_read		= cow_read,
     .bdrv_write		= cow_write,
     .bdrv_close		= cow_close,

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #11 lore raw

PATCH 11/19 block: Move error actions from DriveInfo to BlockDriverState

In reply to Kevin Wolf

From: Markus Armbruster <[email protected]>

That's where they belong semantically (block device host part), even though the actions are actually executed by guest device code.

Signed-off-by Markus Armbruster <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c         |   12 ++++++++++++
 block.h         |    8 ++++++++
 block_int.h     |    1 +
 blockdev.c      |   17 ++---------------
 blockdev.h      |   10 ----------
 hw/ide/core.c   |    2 +-
 hw/scsi-disk.c  |    2 +-
 hw/virtio-blk.c |    3 +--
 8 files changed, 26 insertions(+), 29 deletions(-)

block.c +12 -0

index 29a6f39..e701ec0 100644
@@ -1206,6 +1206,18 @@ int bdrv_get_translation_hint(BlockDriverState *bs)
     return bs->translation;
 }
 
+void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
+                       BlockErrorAction on_write_error)
+{
+    bs->on_read_error = on_read_error;
+    bs->on_write_error = on_write_error;
+}
+
+BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read)
+{
+    return is_read ? bs->on_read_error : bs->on_write_error;
+}
+
 int bdrv_is_removable(BlockDriverState *bs)
 {
     return bs->removable;

block.h +8 -0

index a340ffd..d22401f 100644
@@ -42,6 +42,11 @@ typedef struct QEMUSnapshotInfo {
 #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
 typedef enum {
+    BLOCK_ERR_REPORT, BLOCK_ERR_IGNORE, BLOCK_ERR_STOP_ENOSPC,
+    BLOCK_ERR_STOP_ANY
+} BlockErrorAction;
+
+typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockMonEventAction;

@@ -146,6 +151,9 @@ void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs);
 int bdrv_get_type_hint(BlockDriverState *bs);
 int bdrv_get_translation_hint(BlockDriverState *bs);
+void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
+                       BlockErrorAction on_write_error);
+BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_is_sg(BlockDriverState *bs);

block_int.h +1 -0

index 1a7240c..e3bfd19 100644
@@ -182,6 +182,7 @@ struct BlockDriverState {
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
     int type;
+    BlockErrorAction on_read_error, on_write_error;
     char device_name[32];
     unsigned long *dirty_bitmap;
     int64_t dirty_count;

blockdev.c +2 ~~-15~~

index dbeef09..b5570f4 100644
@@ -90,19 +90,6 @@ const char *drive_get_serial(BlockDriverState *bdrv)
     return "\0";
 }
 
-BlockInterfaceErrorAction drive_get_on_error(
-    BlockDriverState *bdrv, int is_read)
-{
-    DriveInfo *dinfo;
-
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        if (dinfo->bdrv == bdrv)
-            return is_read ? dinfo->on_read_error : dinfo->on_write_error;
-    }
-
-    return is_read ? BLOCK_ERR_REPORT : BLOCK_ERR_STOP_ENOSPC;
-}
-
 static void bdrv_format_print(void *opaque, const char *name)
 {
     fprintf(stderr, " %s", name);

@@ -418,13 +405,13 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     dinfo->type = type;
     dinfo->bus = bus_id;
     dinfo->unit = unit_id;
-    dinfo->on_read_error = on_read_error;
-    dinfo->on_write_error = on_write_error;
     dinfo->opts = opts;
     if (serial)
         strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1);
     QTAILQ_INSERT_TAIL(&drives, dinfo, next);
 
+    bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
+
     switch(type) {
     case IF_IDE:
     case IF_SCSI:

blockdev.h +0 ~~-10~~

index dfc9de1..9e8a7fc 100644
@@ -19,11 +19,6 @@ typedef enum {
     IF_COUNT
 } BlockInterfaceType;
 
-typedef enum {
-    BLOCK_ERR_REPORT, BLOCK_ERR_IGNORE, BLOCK_ERR_STOP_ENOSPC,
-    BLOCK_ERR_STOP_ANY
-} BlockInterfaceErrorAction;
-
 #define BLOCK_SERIAL_STRLEN 20
 
 typedef struct DriveInfo {

@@ -34,8 +29,6 @@ typedef struct DriveInfo {
     int bus;
     int unit;
     QemuOpts *opts;
-    BlockInterfaceErrorAction on_read_error;
-    BlockInterfaceErrorAction on_write_error;
     char serial[BLOCK_SERIAL_STRLEN + 1];
     QTAILQ_ENTRY(DriveInfo) next;
 } DriveInfo;

@@ -51,9 +44,6 @@ extern int drive_get_max_bus(BlockInterfaceType type);
 extern void drive_uninit(DriveInfo *dinfo);
 extern const char *drive_get_serial(BlockDriverState *bdrv);
 
-extern BlockInterfaceErrorAction drive_get_on_error(
-    BlockDriverState *bdrv, int is_read);
-
 extern QemuOpts *drive_add(const char *file, const char *fmt, ...);
 extern DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi,
                              int *fatal_error);

hw/ide/core.c +1 -1

index 045d18d..0b3b7c2 100644
@@ -481,7 +481,7 @@ void ide_dma_error(IDEState *s)
 static int ide_handle_rw_error(IDEState *s, int error, int op)
 {
     int is_read = (op & BM_STATUS_RETRY_READ);
-    BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
+    BlockErrorAction action = bdrv_get_on_error(s->bs, is_read);
 
     if (action == BLOCK_ERR_IGNORE) {
         bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);

hw/scsi-disk.c +1 -1

index a9bf7d2..2b38984 100644
@@ -182,7 +182,7 @@ static void scsi_read_data(SCSIDevice *d, uint32_t tag)
 static int scsi_handle_write_error(SCSIDiskReq *r, int error)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-    BlockInterfaceErrorAction action = drive_get_on_error(s->bs, 0);
+    BlockErrorAction action = bdrv_get_on_error(s->bs, 0);
 
     if (action == BLOCK_ERR_IGNORE) {
         bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);

hw/virtio-blk.c +1 -2

index 4cc94f6..75878eb 100644
@@ -58,8 +58,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     int is_read)
 {
-    BlockInterfaceErrorAction action =
-        drive_get_on_error(req->dev->bs, is_read);
+    BlockErrorAction action = bdrv_get_on_error(req->dev->bs, is_read);
     VirtIOBlock *s = req->dev;
 
     if (action == BLOCK_ERR_IGNORE) {

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #12 lore raw

PATCH 12/19 block: Decouple block device "commit all" from DriveInfo

In reply to Kevin Wolf

From: Markus Armbruster <[email protected]>

do_commit() and mux_proc_byte() iterate over the list of drives defined with drive_init(). This misses host block devices defined by other means. Such means don't exist now, but will be introduced later in this series.

Change them to use new bdrv_commit_all(), which iterates over all host block devices.

Signed-off-by Markus Armbruster <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c     |    9 +++++++++
 block.h     |    1 +
 blockdev.c  |   16 ++++++++--------
 qemu-char.c |    7 +------
 4 files changed, 19 insertions(+), 14 deletions(-)

block.c +9 -0

index e701ec0..df2d3a6 100644
@@ -788,6 +788,15 @@ ro_cleanup:
     return ret;
 }
 
+void bdrv_commit_all(void)
+{
+    BlockDriverState *bs;
+
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        bdrv_commit(bs);
+    }
+}
+
 /*
  * Return values:
  * 0        - success

block.h +1 -0

index d22401f..1f69cbd 100644
@@ -85,6 +85,7 @@ int64_t bdrv_getlength(BlockDriverState *bs);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
 void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int *psecs);
 int bdrv_commit(BlockDriverState *bs);
+void bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);

blockdev.c +8 -8

index b5570f4..d74cd1d 100644
@@ -486,16 +486,16 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
 void do_commit(Monitor *mon, const QDict *qdict)
 {
-    int all_devices;
-    DriveInfo *dinfo;
     const char *device = qdict_get_str(qdict, "device");
+    BlockDriverState *bs;
 
-    all_devices = !strcmp(device, "all");
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        if (!all_devices)
-            if (strcmp(bdrv_get_device_name(dinfo->bdrv), device))
-                continue;
-        bdrv_commit(dinfo->bdrv);
+    if (!strcmp(device, "all")) {
+        bdrv_commit_all();
+    } else {
+        bs = bdrv_find(device);
+        if (bs) {
+            bdrv_commit(bs);
+        }
     }
 }

qemu-char.c +1 -6

index 87628ea..9b69d92 100644
@@ -351,12 +351,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
                  break;
             }
         case 's':
-            {
-                DriveInfo *dinfo;
-                QTAILQ_FOREACH(dinfo, &drives, next) {
-                    bdrv_commit(dinfo->bdrv);
-                }
-            }
+            bdrv_commit_all();
             break;
         case 'b':
             qemu_chr_event(chr, CHR_EVENT_BREAK);

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #13 lore raw

PATCH 13/19 monitor: Make "commit FOO" complain when FOO doesn't exist

In reply to Kevin Wolf

From: Markus Armbruster <[email protected]>

Signed-off-by Markus Armbruster <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 blockdev.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

blockdev.c +4 -2

index d74cd1d..f636bc6 100644
@@ -493,9 +493,11 @@ void do_commit(Monitor *mon, const QDict *qdict)
         bdrv_commit_all();
     } else {
         bs = bdrv_find(device);
-        if (bs) {
-            bdrv_commit(bs);
+        if (!bs) {
+            qerror_report(QERR_DEVICE_NOT_FOUND, device);
+            return;
         }
+        bdrv_commit(bs);
     }
 }

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #14 lore raw

PATCH 14/19 block: New bdrv_next()

In reply to Kevin Wolf

From: Markus Armbruster <[email protected]>

This is a more flexible alternative to bdrv_iterate().

Signed-off-by Markus Armbruster <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c |    8 ++++++++
 block.h |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

block.c +8 -0

index df2d3a6..383d59f 100644
@@ -1330,6 +1330,14 @@ BlockDriverState *bdrv_find(const char *name)
     return NULL;
 }
 
+BlockDriverState *bdrv_next(BlockDriverState *bs)
+{
+    if (!bs) {
+        return QTAILQ_FIRST(&bdrv_states);
+    }
+    return QTAILQ_NEXT(bs, list);
+}
+
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
 {
     BlockDriverState *bs;

block.h +1 -0

index 1f69cbd..9df9b38 100644
@@ -168,6 +168,7 @@ void bdrv_set_change_cb(BlockDriverState *bs,
                         void (*change_cb)(void *opaque), void *opaque);
 void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
 BlockDriverState *bdrv_find(const char *name);
+BlockDriverState *bdrv_next(BlockDriverState *bs);
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
                   void *opaque);
 int bdrv_is_encrypted(BlockDriverState *bs);

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #15 lore raw

PATCH 15/19 block: Decouple savevm from DriveInfo

In reply to Kevin Wolf

From: Markus Armbruster <[email protected]>

We find snapshots by iterating over the list of drives defined with drive_init(). This misses host block devices defined by other means. Such means don't exist now, but will be introduced later in this series.

Iterate over all host block devices instead, with bdrv_next().

Signed-off-by Markus Armbruster <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 savevm.c |   40 ++++++++++++++++++----------------------
 1 files changed, 18 insertions(+), 22 deletions(-)

savevm.c +18 ~~-22~~

index ab58d63..20354a8 100644
@@ -1578,12 +1578,13 @@ out:
 static BlockDriverState *get_bs_snapshots(void)
 {
     BlockDriverState *bs;
-    DriveInfo *dinfo;
 
     if (bs_snapshots)
         return bs_snapshots;
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs = dinfo->bdrv;
+    /* FIXME what if bs_snapshots gets hot-unplugged? */
+
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs)) {
             goto ok;
         }

@@ -1622,12 +1623,11 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
 static int del_existing_snapshots(Monitor *mon, const char *name)
 {
     BlockDriverState *bs;
-    DriveInfo *dinfo;
     QEMUSnapshotInfo sn1, *snapshot = &sn1;
     int ret;
 
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs = dinfo->bdrv;
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {

@@ -1646,7 +1646,6 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
 
 void do_savevm(Monitor *mon, const QDict *qdict)
 {
-    DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
     int ret;

@@ -1661,8 +1660,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     const char *name = qdict_get_try_str(qdict, "name");
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs = dinfo->bdrv;
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
 
         if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
             continue;

@@ -1730,8 +1729,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 
     /* create the snapshots */
 
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs1 = dinfo->bdrv;
+    bs1 = NULL;
+    while ((bs1 = bdrv_next(bs1))) {
         if (bdrv_can_snapshot(bs1)) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);

@@ -1750,15 +1749,14 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 
 int load_vmstate(const char *name)
 {
-    DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs = dinfo->bdrv;
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
 
         if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
             continue;

@@ -1780,8 +1778,8 @@ int load_vmstate(const char *name)
     /* Flush all IO requests so they don't interfere with the new state.  */
     qemu_aio_flush();
 
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs1 = dinfo->bdrv;
+    bs1 = NULL;
+    while ((bs1 = bdrv_next(bs1))) {
         if (bdrv_can_snapshot(bs1)) {
             ret = bdrv_snapshot_goto(bs1, name);
             if (ret < 0) {

@@ -1831,7 +1829,6 @@ int load_vmstate(const char *name)
 
 void do_delvm(Monitor *mon, const QDict *qdict)
 {
-    DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
     int ret;
     const char *name = qdict_get_str(qdict, "name");

@@ -1842,8 +1839,8 @@ void do_delvm(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs1 = dinfo->bdrv;
+    bs1 = NULL;
+    while ((bs1 = bdrv_next(bs1))) {
         if (bdrv_can_snapshot(bs1)) {
             ret = bdrv_snapshot_delete(bs1, name);
             if (ret < 0) {

@@ -1861,7 +1858,6 @@ void do_delvm(Monitor *mon, const QDict *qdict)
 
 void do_info_snapshots(Monitor *mon)
 {
-    DriveInfo *dinfo;
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo *sn_tab, *sn;
     int nb_sns, i;

@@ -1873,8 +1869,8 @@ void do_info_snapshots(Monitor *mon)
         return;
     }
     monitor_printf(mon, "Snapshot devices:");
-    QTAILQ_FOREACH(dinfo, &drives, next) {
-        bs1 = dinfo->bdrv;
+    bs1 = NULL;
+    while ((bs1 = bdrv_next(bs1))) {
         if (bdrv_can_snapshot(bs1)) {
             if (bs == bs1)
                 monitor_printf(mon, " %s", bdrv_get_device_name(bs1));

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #16 lore raw

PATCH 16/19 blockdev: Give drives internal linkage

In reply to Kevin Wolf

From: Markus Armbruster <[email protected]>

This is the list of drives defined with drive_init(). Hide it, so it doesn't get abused.

Signed-off-by Markus Armbruster <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 blockdev.c |    2 +-
 blockdev.h |    2 --
 2 files changed, 1 insertions(+), 3 deletions(-)

blockdev.c +1 -1

index f636bc6..b376884 100644
@@ -15,7 +15,7 @@
 #include "qemu-config.h"
 #include "sysemu.h"
 
-struct drivelist drives = QTAILQ_HEAD_INITIALIZER(drives);
+static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
 QemuOpts *drive_add(const char *file, const char *fmt, ...)
 {

blockdev.h +0 -2

index 9e8a7fc..23ea576 100644
@@ -36,8 +36,6 @@ typedef struct DriveInfo {
 #define MAX_IDE_DEVS	2
 #define MAX_SCSI_DEVS	7
 
-extern QTAILQ_HEAD(drivelist, DriveInfo) drives;
-
 extern DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 extern DriveInfo *drive_get_by_id(const char *id);
 extern int drive_get_max_bus(BlockInterfaceType type);

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #17 lore raw

PATCH 17/19 Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE

In reply to Kevin Wolf

From: Jes Sorensen <[email protected]>

Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE in hw/fdc.c

Per https://bugs.launchpad.net/qemu/+bug/424453 the correct values for FD_CMD_SAVE is 0x2e and FD_CMD_RESTORE is 0x4e. Verified against the Intel 82078 manual which can be found at:
http://wiki.qemu.org/Documentation/HardwareManuals page 22.

Signed-off-by Jes Sorensen <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/fdc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

hw/fdc.c +2 -2

index b978957..45a876d 100644
@@ -369,9 +369,9 @@ enum {
     FD_CMD_PART_ID = 0x18,
     FD_CMD_SCAN_LOW_OR_EQUAL = 0x19,
     FD_CMD_SCAN_HIGH_OR_EQUAL = 0x1d,
-    FD_CMD_SAVE = 0x2c,
+    FD_CMD_SAVE = 0x2e,
     FD_CMD_OPTION = 0x33,
-    FD_CMD_RESTORE = 0x4c,
+    FD_CMD_RESTORE = 0x4e,
     FD_CMD_DRIVE_SPECIFICATION_COMMAND = 0x8e,
     FD_CMD_RELATIVE_SEEK_OUT = 0x8f,
     FD_CMD_FORMAT_AND_WRITE = 0xcd,

Signature · 1 line

1.6.6.1

Anthony Liguori

2010-06-15 14:34 UTC #18 lore raw

[Qemu-devel] Re: [PATCH 17/19] Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE

Quoted 4 lines · Kevin Wolf

From: Jes Sorensen<[email protected]>
Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE in hw/fdc.c

FYI, the subject is duplicated here so the commit log looks a bit funky.

Regards,

Anthony Liguori

Quoted 28 lines

Per https://bugs.launchpad.net/qemu/+bug/424453 the correct values for FD_CMD_SAVE is 0x2e and FD_CMD_RESTORE is 0x4e. Verified against the Intel 82078 manual which can be found at:
http://wiki.qemu.org/Documentation/HardwareManuals page 22.
Signed-off-by Jes Sorensen<[email protected]>
Signed-off-by Kevin Wolf<[email protected]>
---
  hw/fdc.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
hw/fdc.c +2 -2
index b978957..45a876d 100644
@@ -369,9 +369,9 @@ enum {
      FD_CMD_PART_ID = 0x18,
      FD_CMD_SCAN_LOW_OR_EQUAL = 0x19,
      FD_CMD_SCAN_HIGH_OR_EQUAL = 0x1d,
-    FD_CMD_SAVE = 0x2c,
+    FD_CMD_SAVE = 0x2e,
      FD_CMD_OPTION = 0x33,
-    FD_CMD_RESTORE = 0x4c,
+    FD_CMD_RESTORE = 0x4e,
      FD_CMD_DRIVE_SPECIFICATION_COMMAND = 0x8e,
      FD_CMD_RELATIVE_SEEK_OUT = 0x8f,
      FD_CMD_FORMAT_AND_WRITE = 0xcd,

Kevin Wolf

author

2010-06-15 14:42 UTC #19 lore raw

[Qemu-devel] Re: [PATCH 17/19] Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE

Am 15.06.2010 16:28, schrieb Anthony Liguori:

Quoted 7 lines · Kevin Wolf

Quoted 4 lines
From: Jes Sorensen<[email protected]>
Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE in hw/fdc.c
FYI, the subject is duplicated here so the commit log looks a bit funky.

I agree. There are some more patches by Jes in master that look the same, so I didn't complain, but it definitely looks a bit strange.

Kevin

Jes Sorensen

2010-06-15 14:47 UTC #20 lore raw

[Qemu-devel] Re: [PATCH 17/19] Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE

Quoted 11 lines · Kevin Wolf

Am 15.06.2010 16:28, schrieb Anthony Liguori:
Quoted 7 lines · Kevin Wolf
Quoted 4 lines
From: Jes Sorensen<[email protected]>
Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE in hw/fdc.c
FYI, the subject is duplicated here so the commit log looks a bit funky.
I agree. There are some more patches by Jes in master that look the same, so I didn't complain, but it definitely looks a bit strange.

Hmmm maybe I am messing something up. I started with strange duplicate lines because I thought git send-email would cut the first line for the subject line and then just leave the rest in. Probably just me getting used to do it all in git instead of using quilt.

Cheers,
Jes

Kevin Wolf

author

2010-06-15 14:52 UTC #21 lore raw

[Qemu-devel] Re: [PATCH 17/19] Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE

Am 15.06.2010 16:47, schrieb Jes Sorensen:

Quoted 10 lines · Kevin Wolf

Quoted 9 lines
Am 15.06.2010 16:28, schrieb Anthony Liguori:
Quoted 7 lines · Kevin Wolf
Quoted 4 lines
From: Jes Sorensen<[email protected]>
Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE in hw/fdc.c
FYI, the subject is duplicated here so the commit log looks a bit funky.

Quoted 7 lines

Quoted 2 lines
I agree. There are some more patches by Jes in master that look the same, so I didn't complain, but it definitely looks a bit strange.
Hmmm maybe I am messing something up. I started with strange duplicate lines because I thought git send-email would cut the first line for the subject line and then just leave the rest in. Probably just me getting used to do it all in git instead of using quilt.

It does take the first line for the subject, but when importing your mail, git am puts the subject back into the first line, so the whole text should look the same as in your local repository.

Kevin

Jes Sorensen

2010-06-15 14:54 UTC #22 lore raw

[Qemu-devel] Re: [PATCH 17/19] Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE

Quoted 9 lines · Kevin Wolf

Am 15.06.2010 16:47, schrieb Jes Sorensen:
Quoted 4 lines
Hmmm maybe I am messing something up. I started with strange duplicate lines because I thought git send-email would cut the first line for the subject line and then just leave the rest in. Probably just me getting used to do it all in git instead of using quilt.
It does take the first line for the subject, but when importing your mail, git am puts the subject back into the first line, so the whole text should look the same as in your local repository.

I see, I wasn't aware of that ... well I'll try to keep that in mind next time :)

Thanks
Jes

Kevin Wolf

author

2010-06-15 14:20 UTC #23 lore raw

PATCH 18/19 block: fix a warning and possible truncation

In reply to Kevin Wolf

From: Blue Swirl <[email protected]>

Fix a warning from OpenBSD gcc (3.3.5 (propolice)):
/src/qemu/block.c: In function `bdrv_info_stats_bs':
/src/qemu/block.c:1548: warning: long long int format, long unsigned int arg (arg 6)

There may be also truncation effects.

Signed-off-by Blue Swirl <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

block.c +2 -1

index 383d59f..3fc2969 100644
@@ -1574,7 +1574,8 @@ static QObject* bdrv_info_stats_bs(BlockDriverState *bs)
                              "} }",
                              bs->rd_bytes, bs->wr_bytes,
                              bs->rd_ops, bs->wr_ops,
-                             bs->wr_highest_sector * (long)BDRV_SECTOR_SIZE);
+                             bs->wr_highest_sector *
+                             (uint64_t)BDRV_SECTOR_SIZE);
     dict  = qobject_to_qdict(res);
 
     if (*bs->device_name) {

Signature · 1 line

1.6.6.1

Kevin Wolf

author

2010-06-15 14:20 UTC #24 lore raw

PATCH 19/19 xen: Fix build error due to missing include

In reply to Kevin Wolf

From: Jan Kiszka <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

Signed-off-by Kevin Wolf <[email protected]>

---
 hw/xen_backend.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

hw/xen_backend.h +1 -0

index f07f7d4..cc25f9d 100644
@@ -5,6 +5,7 @@
 #include "sysemu.h"
 #include "net.h"
 #include "block_int.h"
+#include "blockdev.h"
 
 /* ------------------------------------------------------------- */

Signature · 1 line

1.6.6.1

Anthony Liguori

2010-06-15 14:26 UTC #25 lore raw

In reply to Kevin Wolf

Quoted 8 lines · Kevin Wolf

The following changes since commit fd42deeb4cb42f90084046e3ebdb4383953195e3:
Gerd Hoffmann (1):
Add exit notifiers.
are available in the git repository at:
git://repo.or.cz/qemu/kevin.git for-anthony

Merged. Thanks.

Regards,

Anthony Liguori

Quoted 55 lines

Blue Swirl (1):
block: fix a warning and possible truncation
Christoph Hellwig (3):
cow: use pread/pwrite
cow: stop using mmap
cow: use qemu block API
Jan Kiszka (1):
xen: Fix build error due to missing include
Jes Sorensen (1):
Correct definitions for FD_CMD_SAVE and FD_CMD_RESTORE
Kevin Wolf (5):
vpc: Read/write multiple sectors at once
qcow2: Allow get_refcount to return errors
qcow2: Allow alloc_clusters_noref to return errors
qcow2: Return real error code in load_refcount_block
qcow2: Restore L1 entry on l2_allocate failure
Markus Armbruster (7):
Fix regression for "-drive file="
block: Move error actions from DriveInfo to BlockDriverState
block: Decouple block device "commit all" from DriveInfo
monitor: Make "commit FOO" complain when FOO doesn't exist
block: New bdrv_next()
block: Decouple savevm from DriveInfo
blockdev: Give drives internal linkage
Miguel Di Ciurcio Filho (1):
savevm: Really verify if a drive supports snapshots
  block.c                |   49 ++++++++++++++++++-
  block.h                |   11 ++++
  block/cow.c            |  127 ++++++++++++++++++++++++++----------------------
  block/qcow2-cluster.c  |    1 +
  block/qcow2-refcount.c |   70 +++++++++++++++++++++++----
  block/vpc.c            |   41 +++++++++++----
  block_int.h            |    1 +
  blockdev.c             |   39 +++++---------
  blockdev.h             |   12 -----
  hw/fdc.c               |    4 +-
  hw/ide/core.c          |    2 +-
  hw/scsi-disk.c         |    2 +-
  hw/virtio-blk.c        |    3 +-
  hw/xen_backend.h       |    1 +
  qemu-char.c            |    7 +--
  qemu-common.h          |    3 -
  qemu-malloc.c          |    5 --
  savevm.c               |   90 +++++++++++++++++++---------------
  18 files changed, 291 insertions(+), 177 deletions(-)