[Qemu-devel] [PATCH v8 00/15] qemu-img map: Allow driver to return file of the allocated block

[Qemu-devel] [PATCH v8 00/15] qemu-img map: Allow driver to return file of the allocated block

[Fam Zheng]

v8: Fix patch 15. [Max]
    Add Max's rev-by in patch 1.

v7: Rebase, update patch 1 for two new bdrv_get_block_status_above() callers in
    qemu-img.c. [Max]
    Add Max's rev-by in patch 12.

Original cover letter
---------------------

I stumbled upon this when looking at external bitmap formats.

Current "qemu-img map" command only displays filename if the data is allocated
in bs (bs->file) itself, or in the backing chain. Otherwise, it displays an
unfriendly error message:

    $ qemu-img create -f vmdk -o subformat=monolithicFlat /tmp/test.vmdk 1G

    $ qemu-img map /tmp/test.vmdk
    Offset          Length          Mapped to       File
    qemu-img: File contains external, encrypted or compressed clusters.

This can be improved. This series extends the .bdrv_co_get_block_status
callback, to let block driver return the BDS of file; then updates all driver
to implement it; and lastly, it changes qemu-img to use this information in
"map" command:


    $ qemu-img map /tmp/test.vmdk
    Offset          Length          Mapped to       File
    0               0x40000000      0               /tmp/test-flat.vmdk

    $ qemu-img map --output json /tmp/test.vmdk
    [{"length": 1073741824, "start": 0, "zero": false, "offset": 0, "depth": 0,
      "file": "/tmp/test-flat.vmdk", "data": true}
    ]


Fam Zheng (15):
  block: Add "file" output parameter to block status query functions
  qcow: Assign bs->file->bs to file in qcow_co_get_block_status
  qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status
  raw: Assign bs to file in raw_co_get_block_status
  iscsi: Assign bs to file in iscsi_co_get_block_status
  parallels: Assign bs->file->bs to file in
    parallels_co_get_block_status
  qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status
  sheepdog: Assign bs to file in sd_co_get_block_status
  vdi: Assign bs->file->bs to file in vdi_co_get_block_status
  vpc: Assign bs->file->bs to file in vpc_co_get_block_status
  vmdk: Return extent's file in bdrv_get_block_status
  block: Use returned *file in bdrv_co_get_block_status
  qemu-img: In "map", use the returned "file" from bdrv_get_block_status
  qemu-img: Make MapEntry a QAPI struct
  iotests: Add "qemu-img map" test for VMDK extents

 block/io.c                 | 42 +++++++++++++++--------
 block/iscsi.c              |  9 +++--
 block/mirror.c             |  3 +-
 block/parallels.c          |  3 +-
 block/qcow.c               |  3 +-
 block/qcow2.c              |  3 +-
 block/qed.c                |  6 +++-
 block/raw-posix.c          |  4 ++-
 block/raw_bsd.c            |  4 ++-
 block/sheepdog.c           |  5 ++-
 block/vdi.c                |  3 +-
 block/vmdk.c               | 12 ++++---
 block/vpc.c                |  4 ++-
 block/vvfat.c              |  2 +-
 include/block/block.h      | 11 +++---
 include/block/block_int.h  |  3 +-
 qapi/block-core.json       | 27 +++++++++++++++
 qemu-img.c                 | 84 ++++++++++++++++++++++++++++------------------
 tests/qemu-iotests/059     | 10 ++++++
 tests/qemu-iotests/059.out | 26 ++++++++++++++
 20 files changed, 194 insertions(+), 70 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v8 01/15] block: Add "file" output parameter to block status query functions

[Fam Zheng]

The added parameter can be used to return the BDS pointer which the
valid offset is referring to. Its value should be ignored unless
BDRV_BLOCK_OFFSET_VALID in ret is set.

Until block drivers fill in the right value, let's clear it explicitly
right before calling .bdrv_get_block_status.

The "bs->file" condition in bdrv_co_get_block_status is kept now to keep iotest
case 102 passing, and will be fixed once all drivers return the right file
pointer.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/io.c                | 38 ++++++++++++++++++++++++++------------
 block/iscsi.c             |  6 ++++--
 block/mirror.c            |  3 ++-
 block/parallels.c         |  2 +-
 block/qcow.c              |  2 +-
 block/qcow2.c             |  2 +-
 block/qed.c               |  3 ++-
 block/raw-posix.c         |  3 ++-
 block/raw_bsd.c           |  3 ++-
 block/sheepdog.c          |  2 +-
 block/vdi.c               |  2 +-
 block/vmdk.c              |  2 +-
 block/vpc.c               |  2 +-
 block/vvfat.c             |  2 +-
 include/block/block.h     | 11 +++++++----
 include/block/block_int.h |  3 ++-
 qemu-img.c                | 13 +++++++++----
 17 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/block/io.c b/block/io.c
index 5bb353a..0836991 100644
--- a/block/io.c
+++ b/block/io.c
@@ -664,6 +664,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
 int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
 {
     int64_t target_sectors, ret, nb_sectors, sector_num = 0;
+    BlockDriverState *file;
     int n;
 
     target_sectors = bdrv_nb_sectors(bs);
@@ -676,7 +677,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
         if (nb_sectors <= 0) {
             return 0;
         }
-        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
+        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
         if (ret < 0) {
             error_report("error getting block status at sector %" PRId64 ": %s",
                          sector_num, strerror(-ret));
@@ -1466,6 +1467,7 @@ int bdrv_flush_all(void)
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
+    BlockDriverState **file;
     int64_t sector_num;
     int nb_sectors;
     int *pnum;
@@ -1490,7 +1492,8 @@ typedef struct BdrvCoGetBlockStatusData {
  */
 static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
                                                      int64_t sector_num,
-                                                     int nb_sectors, int *pnum)
+                                                     int nb_sectors, int *pnum,
+                                                     BlockDriverState **file)
 {
     int64_t total_sectors;
     int64_t n;
@@ -1520,16 +1523,19 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         return ret;
     }
 
-    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
+    *file = NULL;
+    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
+                                            file);
     if (ret < 0) {
         *pnum = 0;
+        *file = NULL;
         return ret;
     }
 
     if (ret & BDRV_BLOCK_RAW) {
         assert(ret & BDRV_BLOCK_OFFSET_VALID);
         return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
-                                     *pnum, pnum);
+                                     *pnum, pnum, file);
     }
 
     if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
@@ -1549,10 +1555,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     if (bs->file &&
         (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
         (ret & BDRV_BLOCK_OFFSET_VALID)) {
+        BlockDriverState *file2;
         int file_pnum;
 
         ret2 = bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
-                                        *pnum, &file_pnum);
+                                        *pnum, &file_pnum, &file2);
         if (ret2 >= 0) {
             /* Ignore errors.  This is just providing extra information, it
              * is useful but not necessary.
@@ -1577,14 +1584,15 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
         BlockDriverState *base,
         int64_t sector_num,
         int nb_sectors,
-        int *pnum)
+        int *pnum,
+        BlockDriverState **file)
 {
     BlockDriverState *p;
     int64_t ret = 0;
 
     assert(bs != base);
     for (p = bs; p != base; p = backing_bs(p)) {
-        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum);
+        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
         if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
             break;
         }
@@ -1603,7 +1611,8 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
     data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
                                                data->sector_num,
                                                data->nb_sectors,
-                                               data->pnum);
+                                               data->pnum,
+                                               data->file);
     data->done = true;
 }
 
@@ -1615,12 +1624,14 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
 int64_t bdrv_get_block_status_above(BlockDriverState *bs,
                                     BlockDriverState *base,
                                     int64_t sector_num,
-                                    int nb_sectors, int *pnum)
+                                    int nb_sectors, int *pnum,
+                                    BlockDriverState **file)
 {
     Coroutine *co;
     BdrvCoGetBlockStatusData data = {
         .bs = bs,
         .base = base,
+        .file = file,
         .sector_num = sector_num,
         .nb_sectors = nb_sectors,
         .pnum = pnum,
@@ -1644,16 +1655,19 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
 
 int64_t bdrv_get_block_status(BlockDriverState *bs,
                               int64_t sector_num,
-                              int nb_sectors, int *pnum)
+                              int nb_sectors, int *pnum,
+                              BlockDriverState **file)
 {
     return bdrv_get_block_status_above(bs, backing_bs(bs),
-                                       sector_num, nb_sectors, pnum);
+                                       sector_num, nb_sectors, pnum, file);
 }
 
 int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
                                    int nb_sectors, int *pnum)
 {
-    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum);
+    BlockDriverState *file;
+    int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum,
+                                        &file);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/iscsi.c b/block/iscsi.c
index bffd707..e182557 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -532,7 +532,8 @@ static bool iscsi_allocationmap_is_allocated(IscsiLun *iscsilun,
 
 static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
                                                   int64_t sector_num,
-                                                  int nb_sectors, int *pnum)
+                                                  int nb_sectors, int *pnum,
+                                                  BlockDriverState **file)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct scsi_get_lba_status *lbas = NULL;
@@ -650,7 +651,8 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
         !iscsi_allocationmap_is_allocated(iscsilun, sector_num, nb_sectors)) {
         int64_t ret;
         int pnum;
-        ret = iscsi_co_get_block_status(bs, sector_num, INT_MAX, &pnum);
+        BlockDriverState *file;
+        ret = iscsi_co_get_block_status(bs, sector_num, INT_MAX, &pnum, &file);
         if (ret < 0) {
             return ret;
         }
diff --git a/block/mirror.c b/block/mirror.c
index e9e151c..2c0edfa 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -168,6 +168,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     MirrorOp *op;
     int pnum;
     int64_t ret;
+    BlockDriverState *file;
 
     max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov);
 
@@ -306,7 +307,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     trace_mirror_one_iteration(s, sector_num, nb_sectors);
 
     ret = bdrv_get_block_status_above(source, NULL, sector_num,
-                                      nb_sectors, &pnum);
+                                      nb_sectors, &pnum, &file);
     if (ret < 0 || pnum < nb_sectors ||
             (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
         bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
diff --git a/block/parallels.c b/block/parallels.c
index ee39081..e2de308 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -261,7 +261,7 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs)
 
 
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum)
+        int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
 {
     BDRVParallelsState *s = bs->opaque;
     int64_t offset;
diff --git a/block/qcow.c b/block/qcow.c
index afed18f..4202797 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -489,7 +489,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
 }
 
 static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum)
+        int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
 {
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster, n;
diff --git a/block/qcow2.c b/block/qcow2.c
index fd8436c..d4ea6b4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1330,7 +1330,7 @@ static void qcow2_join_options(QDict *options, QDict *old_options)
 }
 
 static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum)
+        int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t cluster_offset;
diff --git a/block/qed.c b/block/qed.c
index 0c870cd..8f6f841 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -725,7 +725,8 @@ static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t l
 
 static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
                                                  int64_t sector_num,
-                                                 int nb_sectors, int *pnum)
+                                                 int nb_sectors, int *pnum,
+                                                 BlockDriverState **file)
 {
     BDRVQEDState *s = bs->opaque;
     size_t len = (size_t)nb_sectors * BDRV_SECTOR_SIZE;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6df3067..3ef9b25 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1818,7 +1818,8 @@ static int find_allocation(BlockDriverState *bs, off_t start,
  */
 static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                                     int64_t sector_num,
-                                                    int nb_sectors, int *pnum)
+                                                    int nb_sectors, int *pnum,
+                                                    BlockDriverState **file)
 {
     off_t start, data = 0, hole = 0;
     int64_t total_size;
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index bcaee11..9a8933b 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -115,7 +115,8 @@ fail:
 
 static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                             int64_t sector_num,
-                                            int nb_sectors, int *pnum)
+                                            int nb_sectors, int *pnum,
+                                            BlockDriverState **file)
 {
     *pnum = nb_sectors;
     return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
diff --git a/block/sheepdog.c b/block/sheepdog.c
index ff89298..2ea05a6 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2708,7 +2708,7 @@ retry:
 
 static coroutine_fn int64_t
 sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
-                       int *pnum)
+                       int *pnum, BlockDriverState **file)
 {
     BDRVSheepdogState *s = bs->opaque;
     SheepdogInode *inode = &s->inode;
diff --git a/block/vdi.c b/block/vdi.c
index 61bcd54..294c438 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -527,7 +527,7 @@ static int vdi_reopen_prepare(BDRVReopenState *state,
 }
 
 static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum)
+        int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
 {
     /* TODO: Check for too large sector_num (in bdrv_is_allocated or here). */
     BDRVVdiState *s = (BDRVVdiState *)bs->opaque;
diff --git a/block/vmdk.c b/block/vmdk.c
index 698679d..e1d3e27 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1257,7 +1257,7 @@ static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
 }
 
 static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum)
+        int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
 {
     BDRVVmdkState *s = bs->opaque;
     int64_t index_in_cluster, n, ret;
diff --git a/block/vpc.c b/block/vpc.c
index d852f96..a070307 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -579,7 +579,7 @@ static coroutine_fn int vpc_co_write(BlockDriverState *bs, int64_t sector_num,
 }
 
 static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum)
+        int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
 {
     BDRVVPCState *s = bs->opaque;
     VHDFooter *footer = (VHDFooter*) s->footer_buf;
diff --git a/block/vvfat.c b/block/vvfat.c
index 2ea5a4a..b8d29e1 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2884,7 +2884,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
 }
 
 static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
-	int64_t sector_num, int nb_sectors, int* n)
+	int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
 {
     BDRVVVFATState* s = bs->opaque;
     *n = s->sector_count - sector_num;
diff --git a/include/block/block.h b/include/block/block.h
index 25f36dc..ee845a9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -111,9 +111,10 @@ typedef struct HDGeometry {
 
 /*
  * Allocation status flags
- * BDRV_BLOCK_DATA: data is read from bs->file or another file
+ * BDRV_BLOCK_DATA: data is read from a file returned by bdrv_get_block_status.
  * BDRV_BLOCK_ZERO: sectors read as zero
- * BDRV_BLOCK_OFFSET_VALID: sector stored in bs->file as raw data
+ * BDRV_BLOCK_OFFSET_VALID: sector stored as raw data in a file returned by
+ *                          bdrv_get_block_status.
  * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
  *                       layer (as opposed to the backing file)
  * BDRV_BLOCK_RAW: used internally to indicate that the request
@@ -386,11 +387,13 @@ int bdrv_has_zero_init(BlockDriverState *bs);
 bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
-                              int nb_sectors, int *pnum);
+                              int nb_sectors, int *pnum,
+                              BlockDriverState **file);
 int64_t bdrv_get_block_status_above(BlockDriverState *bs,
                                     BlockDriverState *base,
                                     int64_t sector_num,
-                                    int nb_sectors, int *pnum);
+                                    int nb_sectors, int *pnum,
+                                    BlockDriverState **file);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
                       int *pnum);
 int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 428fa33..5fa58e8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -166,7 +166,8 @@ struct BlockDriver {
     int coroutine_fn (*bdrv_co_discard)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors);
     int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
-        int64_t sector_num, int nb_sectors, int *pnum);
+        int64_t sector_num, int nb_sectors, int *pnum,
+        BlockDriverState **file);
 
     /*
      * Invalidate any cached meta-data.
diff --git a/qemu-img.c b/qemu-img.c
index 33e451c..e653b2f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1072,13 +1072,15 @@ static int img_compare(int argc, char **argv)
 
     for (;;) {
         int64_t status1, status2;
+        BlockDriverState *file;
+
         nb_sectors = sectors_to_process(total_sectors, sector_num);
         if (nb_sectors <= 0) {
             break;
         }
         status1 = bdrv_get_block_status_above(bs1, NULL, sector_num,
                                               total_sectors1 - sector_num,
-                                              &pnum1);
+                                              &pnum1, &file);
         if (status1 < 0) {
             ret = 3;
             error_report("Sector allocation test failed for %s", filename1);
@@ -1088,7 +1090,7 @@ static int img_compare(int argc, char **argv)
 
         status2 = bdrv_get_block_status_above(bs2, NULL, sector_num,
                                               total_sectors2 - sector_num,
-                                              &pnum2);
+                                              &pnum2, &file);
         if (status2 < 0) {
             ret = 3;
             error_report("Sector allocation test failed for %s", filename2);
@@ -1271,9 +1273,10 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
     n = MIN(s->total_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS);
 
     if (s->sector_next_status <= sector_num) {
+        BlockDriverState *file;
         ret = bdrv_get_block_status(blk_bs(s->src[s->src_cur]),
                                     sector_num - s->src_cur_offset,
-                                    n, &n);
+                                    n, &n, &file);
         if (ret < 0) {
             return ret;
         }
@@ -2201,6 +2204,7 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
 {
     int64_t ret;
     int depth;
+    BlockDriverState *file;
 
     /* As an optimization, we could cache the current range of unallocated
      * clusters in each file of the chain, and avoid querying the same
@@ -2209,7 +2213,8 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
 
     depth = 0;
     for (;;) {
-        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &nb_sectors);
+        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &nb_sectors,
+                                    &file);
         if (ret < 0) {
             return ret;
         }
-- 
2.4.3

[Qemu-devel] [PATCH v8 02/15] qcow: Assign bs->file->bs to file in qcow_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qcow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow.c b/block/qcow.c
index 4202797..251910c 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -510,6 +510,7 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
         return BDRV_BLOCK_DATA;
     }
     cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+    *file = bs->file->bs;
     return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | cluster_offset;
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v8 03/15] qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status

[Fam Zheng]

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index d4ea6b4..8babecd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1349,6 +1349,7 @@ static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
         !s->cipher) {
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
         cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+        *file = bs->file->bs;
         status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
     }
     if (ret == QCOW2_CLUSTER_ZERO) {
-- 
2.4.3

[Qemu-devel] [PATCH v8 04/15] raw: Assign bs to file in raw_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 1 +
 block/raw_bsd.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 3ef9b25..8866121 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1861,6 +1861,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
         *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
         ret = BDRV_BLOCK_ZERO;
     }
+    *file = bs;
     return ret | BDRV_BLOCK_OFFSET_VALID | start;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 9a8933b..fd355d5 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -119,6 +119,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
                                             BlockDriverState **file)
 {
     *pnum = nb_sectors;
+    *file = bs;
     return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
            (sector_num << BDRV_SECTOR_BITS);
 }
-- 
2.4.3

[Qemu-devel] [PATCH v8 05/15] iscsi: Assign bs to file in iscsi_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/iscsi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index e182557..9fe76f4 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -625,6 +625,9 @@ out:
     if (iTask.task != NULL) {
         scsi_free_scsi_task(iTask.task);
     }
+    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+        *file = bs;
+    }
     return ret;
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v8 06/15] parallels: Assign bs->file->bs to file in parallels_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/parallels.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels.c b/block/parallels.c
index e2de308..645521d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -274,6 +274,7 @@ static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         return 0;
     }
 
+    *file = bs->file->bs;
     return (offset << BDRV_SECTOR_BITS) |
         BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
 }
-- 
2.4.3

[Qemu-devel] [PATCH v8 07/15] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qed.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/qed.c b/block/qed.c
index 8f6f841..404be1e 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -693,6 +693,7 @@ typedef struct {
     uint64_t pos;
     int64_t status;
     int *pnum;
+    BlockDriverState **file;
 } QEDIsAllocatedCB;
 
 static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t len)
@@ -704,6 +705,7 @@ static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t l
     case QED_CLUSTER_FOUND:
         offset |= qed_offset_into_cluster(s, cb->pos);
         cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+        *cb->file = cb->bs->file->bs;
         break;
     case QED_CLUSTER_ZERO:
         cb->status = BDRV_BLOCK_ZERO;
@@ -735,6 +737,7 @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
         .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
         .status = BDRV_BLOCK_OFFSET_MASK,
         .pnum = pnum,
+        .file = file,
     };
     QEDRequest request = { .l2_table = NULL };
 
-- 
2.4.3

[Qemu-devel] [PATCH v8 08/15] sheepdog: Assign bs to file in sd_co_get_block_status

[Fam Zheng]

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/sheepdog.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 2ea05a6..a0098c1 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2739,6 +2739,9 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     if (*pnum > nb_sectors) {
         *pnum = nb_sectors;
     }
+    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+        *file = bs;
+    }
     return ret;
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v8 09/15] vdi: Assign bs->file->bs to file in vdi_co_get_block_status

[Fam Zheng]

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vdi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vdi.c b/block/vdi.c
index 294c438..b403243 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -551,6 +551,7 @@ static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs,
     offset = s->header.offset_data +
                               (uint64_t)bmap_entry * s->block_size +
                               sector_in_block * SECTOR_SIZE;
+    *file = bs->file->bs;
     return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v8 10/15] vpc: Assign bs->file->bs to file in vpc_co_get_block_status

[Fam Zheng]

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vpc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index a070307..f504536 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -589,6 +589,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
 
     if (be32_to_cpu(footer->type) == VHD_FIXED) {
         *pnum = nb_sectors;
+        *file = bs->file->bs;
         return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
                (sector_num << BDRV_SECTOR_BITS);
     }
@@ -610,6 +611,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
         /* *pnum can't be greater than one block for allocated
          * sectors since there is always a bitmap in between. */
         if (allocated) {
+            *file = bs->file->bs;
             return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
         }
         if (nb_sectors == 0) {
-- 
2.4.3

[Qemu-devel] [PATCH v8 11/15] vmdk: Return extent's file in bdrv_get_block_status

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/vmdk.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index e1d3e27..f8f7fcf 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1274,6 +1274,7 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
                              0, 0);
     qemu_co_mutex_unlock(&s->lock);
 
+    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
     switch (ret) {
     case VMDK_ERROR:
         ret = -EIO;
@@ -1286,14 +1287,15 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
         break;
     case VMDK_OK:
         ret = BDRV_BLOCK_DATA;
-        if (extent->file == bs->file && !extent->compressed) {
-            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
+        if (!extent->compressed) {
+            ret |= BDRV_BLOCK_OFFSET_VALID;
+            ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
+                    & BDRV_BLOCK_OFFSET_MASK;
         }
-
+        *file = extent->file->bs;
         break;
     }
 
-    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
     n = extent->cluster_sectors - index_in_cluster;
     if (n > nb_sectors) {
         n = nb_sectors;
-- 
2.4.3

[Qemu-devel] [PATCH v8 12/15] block: Use returned *file in bdrv_co_get_block_status

[Fam Zheng]

Now that all drivers return the right "file" pointer, we can use it.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 0836991..d704d32 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1552,13 +1552,13 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         }
     }
 
-    if (bs->file &&
+    if (*file && *file != bs &&
         (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
         (ret & BDRV_BLOCK_OFFSET_VALID)) {
         BlockDriverState *file2;
         int file_pnum;
 
-        ret2 = bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
+        ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
                                         *pnum, &file_pnum, &file2);
         if (ret2 >= 0) {
             /* Ignore errors.  This is just providing extra information, it
-- 
2.4.3

[Qemu-devel] [PATCH v8 13/15] qemu-img: In "map", use the returned "file" from bdrv_get_block_status

[Fam Zheng]

Now all drivers should return a correct "file", we can make use of it,
even with the recursion into backing chain above.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index e653b2f..c8bc63f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2236,7 +2236,7 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
     e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
     e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
     e->depth = depth;
-    e->bs = bs;
+    e->bs = file;
     return 0;
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v8 14/15] qemu-img: Make MapEntry a QAPI struct

[Fam Zheng]

The "flags" bit mask is expanded to two booleans, "data" and "zero";
"bs" is replaced with "filename" string.

Refactor the merge conditions in img_map() into entry_mergeable().

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi/block-core.json | 27 ++++++++++++++++++++
 qemu-img.c           | 71 +++++++++++++++++++++++++++++++---------------------
 2 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0a915ed..30c2e5f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -186,6 +186,33 @@
            '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
 
 ##
+# @MapEntry:
+#
+# Mapping information from a virtual block range to a host file range
+#
+# @start: the start byte of the mapped virtual range
+#
+# @length: the number of bytes of the mapped virtual range
+#
+# @data: whether the mapped range has data
+#
+# @zero: whether the virtual blocks are zeroed
+#
+# @depth: the depth of the mapping
+#
+# @offset: #optional the offset in file that the virtual sectors are mapped to
+#
+# @filename: #optional filename that is referred to by @offset
+#
+# Since: 2.6
+#
+##
+{ 'struct': 'MapEntry',
+  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
+           'zero': 'bool', 'depth': 'int', '*offset': 'int',
+           '*filename': 'str' } }
+
+##
 # @BlockdevCacheInfo
 #
 # Cache mode information for a block device
diff --git a/qemu-img.c b/qemu-img.c
index c8bc63f..f121980 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2147,47 +2147,37 @@ static int img_info(int argc, char **argv)
     return 0;
 }
 
-
-typedef struct MapEntry {
-    int flags;
-    int depth;
-    int64_t start;
-    int64_t length;
-    int64_t offset;
-    BlockDriverState *bs;
-} MapEntry;
-
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
                            MapEntry *next)
 {
     switch (output_format) {
     case OFORMAT_HUMAN:
-        if ((e->flags & BDRV_BLOCK_DATA) &&
-            !(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
+        if (e->data && !e->has_offset) {
             error_report("File contains external, encrypted or compressed clusters.");
             exit(1);
         }
-        if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
+        if (e->data && !e->zero) {
             printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
-                   e->start, e->length, e->offset, e->bs->filename);
+                   e->start, e->length,
+                   e->has_offset ? e->offset : 0,
+                   e->has_filename ? e->filename : "");
         }
         /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
          * Modify the flags here to allow more coalescing.
          */
-        if (next &&
-            (next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) != BDRV_BLOCK_DATA) {
-            next->flags &= ~BDRV_BLOCK_DATA;
-            next->flags |= BDRV_BLOCK_ZERO;
+        if (next && (!next->data || next->zero)) {
+            next->data = false;
+            next->zero = true;
         }
         break;
     case OFORMAT_JSON:
-        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
-               " \"zero\": %s, \"data\": %s",
+        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
+               " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
                (e->start == 0 ? "[" : ",\n"),
                e->start, e->length, e->depth,
-               (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false",
-               (e->flags & BDRV_BLOCK_DATA) ? "true" : "false");
-        if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
+               e->zero ? "true" : "false",
+               e->data ? "true" : "false");
+        if (e->has_offset) {
             printf(", \"offset\": %"PRId64"", e->offset);
         }
         putchar('}');
@@ -2233,13 +2223,39 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
 
     e->start = sector_num * BDRV_SECTOR_SIZE;
     e->length = nb_sectors * BDRV_SECTOR_SIZE;
-    e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
+    e->data = !!(ret & BDRV_BLOCK_DATA);
+    e->zero = !!(ret & BDRV_BLOCK_ZERO);
     e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
+    e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
     e->depth = depth;
-    e->bs = file;
+    if (file && e->has_offset) {
+        e->has_filename = true;
+        e->filename = file->filename;
+    }
     return 0;
 }
 
+static inline bool entry_mergeable(const MapEntry *curr, const MapEntry *next)
+{
+    if (curr->length == 0) {
+        return false;
+    }
+    if (curr->zero != next->zero ||
+        curr->data != next->data ||
+        curr->depth != next->depth ||
+        curr->has_filename != next->has_filename ||
+        curr->has_offset != next->has_offset) {
+        return false;
+    }
+    if (curr->has_filename && strcmp(curr->filename, next->filename)) {
+        return false;
+    }
+    if (curr->has_offset && curr->offset + curr->length != next->offset) {
+        return false;
+    }
+    return true;
+}
+
 static int img_map(int argc, char **argv)
 {
     int c;
@@ -2321,10 +2337,7 @@ static int img_map(int argc, char **argv)
             goto out;
         }
 
-        if (curr.length != 0 && curr.flags == next.flags &&
-            curr.depth == next.depth &&
-            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
-             curr.offset + curr.length == next.offset)) {
+        if (entry_mergeable(&curr, &next)) {
             curr.length += next.length;
             continue;
         }
-- 
2.4.3

[Qemu-devel] [PATCH v8 15/15] iotests: Add "qemu-img map" test for VMDK extents

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/059     | 10 ++++++++++
 tests/qemu-iotests/059.out | 26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 0ded0c3..0332bbb 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -133,6 +133,16 @@ $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
 
 echo
+echo "=== Testing qemu-img map on extents ==="
+for fmt in monolithicSparse twoGbMaxExtentSparse; do
+    IMGOPTS="subformat=$fmt" _make_test_img 31G
+    $QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io
+    $QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io
+    $QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io
+    $QEMU_IMG map "$TEST_IMG" | _filter_testdir
+done
+
+echo
 echo "=== Testing afl image with a very large capacity ==="
 _use_sample_img afl9.vmdk.bz2
 _img_info
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 9d506cb..5e041d7 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2050,6 +2050,7 @@ wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 512/512 bytes at offset 10240
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: Could not open '/home/fam/build/last/tests/qemu-iotests/scratch/t.vmdk': VMDK version 3 must be read only
 
 === Testing monolithicFlat with internally generated JSON file name ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 subformat=monolithicFlat
@@ -2335,6 +2336,31 @@ e1000003f0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
 read 1024/1024 bytes at offset 966367641600
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+=== Testing qemu-img map on extents ===
+Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=monolithicSparse
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 2147483136
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 5368709120
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          Mapped to       File
+0               0x20000         0x3f0000        TEST_DIR/iotest-version3.vmdk
+0x7fff0000      0x20000         0x410000        TEST_DIR/iotest-version3.vmdk
+0x140000000     0x10000         0x430000        TEST_DIR/iotest-version3.vmdk
+Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=twoGbMaxExtentSparse
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 2147483136
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 5368709120
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          Mapped to       File
+0               0x20000         0x50000         TEST_DIR/iotest-version3-s001.vmdk
+0x7fff0000      0x10000         0x70000         TEST_DIR/iotest-version3-s001.vmdk
+0x80000000      0x10000         0x50000         TEST_DIR/iotest-version3-s002.vmdk
+0x140000000     0x10000         0x50000         TEST_DIR/iotest-version3-s003.vmdk
+
 === Testing afl image with a very large capacity ===
 qemu-img: Can't get size of device 'image': File too large
 *** done
-- 
2.4.3

Re: [Qemu-devel] [PATCH v8 01/15] block: Add "file" output parameter to block status query functions

[Kevin Wolf]

Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> The added parameter can be used to return the BDS pointer which the
> valid offset is referring to. Its value should be ignored unless
> BDRV_BLOCK_OFFSET_VALID in ret is set.
> 
> Until block drivers fill in the right value, let's clear it explicitly
> right before calling .bdrv_get_block_status.
> 
> The "bs->file" condition in bdrv_co_get_block_status is kept now to keep iotest
> case 102 passing, and will be fixed once all drivers return the right file
> pointer.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c                | 38 ++++++++++++++++++++++++++------------
>  block/iscsi.c             |  6 ++++--
>  block/mirror.c            |  3 ++-
>  block/parallels.c         |  2 +-
>  block/qcow.c              |  2 +-
>  block/qcow2.c             |  2 +-
>  block/qed.c               |  3 ++-
>  block/raw-posix.c         |  3 ++-
>  block/raw_bsd.c           |  3 ++-
>  block/sheepdog.c          |  2 +-
>  block/vdi.c               |  2 +-
>  block/vmdk.c              |  2 +-
>  block/vpc.c               |  2 +-
>  block/vvfat.c             |  2 +-
>  include/block/block.h     | 11 +++++++----
>  include/block/block_int.h |  3 ++-
>  qemu-img.c                | 13 +++++++++----
>  17 files changed, 64 insertions(+), 35 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 5bb353a..0836991 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -664,6 +664,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>  int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>  {
>      int64_t target_sectors, ret, nb_sectors, sector_num = 0;
> +    BlockDriverState *file;
>      int n;
>  
>      target_sectors = bdrv_nb_sectors(bs);
> @@ -676,7 +677,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>          if (nb_sectors <= 0) {
>              return 0;
>          }
> -        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
>          if (ret < 0) {
>              error_report("error getting block status at sector %" PRId64 ": %s",
>                           sector_num, strerror(-ret));
> @@ -1466,6 +1467,7 @@ int bdrv_flush_all(void)
>  typedef struct BdrvCoGetBlockStatusData {
>      BlockDriverState *bs;
>      BlockDriverState *base;
> +    BlockDriverState **file;
>      int64_t sector_num;
>      int nb_sectors;
>      int *pnum;
> @@ -1490,7 +1492,8 @@ typedef struct BdrvCoGetBlockStatusData {
>   */
>  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int64_t sector_num,
> -                                                     int nb_sectors, int *pnum)
> +                                                     int nb_sectors, int *pnum,
> +                                                     BlockDriverState **file)

This function has a comment explaining all parameters. Except **file
now.

My impression at the moment is that it generally returns bs->file for
format drivers and bs itself for protocol drivers if the sector is
allocated in this layer.

>  {
>      int64_t total_sectors;
>      int64_t n;
> @@ -1520,16 +1523,19 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>          return ret;
>      }
>  
> -    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> +    *file = NULL;
> +    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
> +                                            file);
>      if (ret < 0) {
>          *pnum = 0;
> +        *file = NULL;

You specified that file is to be ignored unless BDRV_BLOCK_OFFSET_VALID
is set. Why reset it here then? It's not reset in other error cases.

>          return ret;
>      }
>  
>      if (ret & BDRV_BLOCK_RAW) {
>          assert(ret & BDRV_BLOCK_OFFSET_VALID);
>          return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
> -                                     *pnum, pnum);
> +                                     *pnum, pnum, file);
>      }
>  
>      if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {

> diff --git a/block/vvfat.c b/block/vvfat.c
> index 2ea5a4a..b8d29e1 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2884,7 +2884,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
>  }
>  
>  static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
> -	int64_t sector_num, int nb_sectors, int* n)
> +	int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
>  {
>      BDRVVVFATState* s = bs->opaque;
>      *n = s->sector_count - sector_num;

This still returns NULL at the end of the series. Shouldn't it return bs
like other protocol drivers do?

Kevin

Re: [Qemu-devel] [PATCH v8 04/15] raw: Assign bs to file in raw_co_get_block_status

[Kevin Wolf]

Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 1 +
>  block/raw_bsd.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 3ef9b25..8866121 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1861,6 +1861,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>          *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
>          ret = BDRV_BLOCK_ZERO;
>      }
> +    *file = bs;
>      return ret | BDRV_BLOCK_OFFSET_VALID | start;
>  }
>  
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 9a8933b..fd355d5 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -119,6 +119,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>                                              BlockDriverState **file)
>  {
>      *pnum = nb_sectors;
> +    *file = bs;

This should be bs->file->bs.

>      return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
>             (sector_num << BDRV_SECTOR_BITS);
>  }

Kevin

Re: [Qemu-devel] [PATCH v8 11/15] vmdk: Return extent's file in bdrv_get_block_status

[Kevin Wolf]

Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e1d3e27..f8f7fcf 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1274,6 +1274,7 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
>                               0, 0);
>      qemu_co_mutex_unlock(&s->lock);
>  
> +    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
>      switch (ret) {
>      case VMDK_ERROR:
>          ret = -EIO;
> @@ -1286,14 +1287,15 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
>          break;
>      case VMDK_OK:
>          ret = BDRV_BLOCK_DATA;
> -        if (extent->file == bs->file && !extent->compressed) {
> -            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
> +        if (!extent->compressed) {
> +            ret |= BDRV_BLOCK_OFFSET_VALID;
> +            ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
> +                    & BDRV_BLOCK_OFFSET_MASK;
>          }
> -
> +        *file = extent->file->bs;
>          break;
>      }
>  
> -    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
>      n = extent->cluster_sectors - index_in_cluster;
>      if (n > nb_sectors) {
>          n = nb_sectors;

The commit message made me expect that this does exactly the same as the
other patches, i.e. fill the file argument and nothing else.

Instead, this extends the functioniality to work on any kind of extents
instead of just the the main VMDK file, and it changes the calculation
of the offset (seems to be a hidden bug fix?)

This needs a non-empty commit message, and depending on what you're
going to write there, possibly splitting the patch.

Kevin

Re: [Qemu-devel] [PATCH v8 00/15] qemu-img map: Allow driver to return file of the allocated block

[Kevin Wolf]

Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> v8: Fix patch 15. [Max]
>     Add Max's rev-by in patch 1.
> 
> v7: Rebase, update patch 1 for two new bdrv_get_block_status_above() callers in
>     qemu-img.c. [Max]
>     Add Max's rev-by in patch 12.
> 
> Original cover letter
> ---------------------
> 
> I stumbled upon this when looking at external bitmap formats.
> 
> Current "qemu-img map" command only displays filename if the data is allocated
> in bs (bs->file) itself, or in the backing chain. Otherwise, it displays an
> unfriendly error message:
> 
>     $ qemu-img create -f vmdk -o subformat=monolithicFlat /tmp/test.vmdk 1G
> 
>     $ qemu-img map /tmp/test.vmdk
>     Offset          Length          Mapped to       File
>     qemu-img: File contains external, encrypted or compressed clusters.
> 
> This can be improved. This series extends the .bdrv_co_get_block_status
> callback, to let block driver return the BDS of file; then updates all driver
> to implement it; and lastly, it changes qemu-img to use this information in
> "map" command:
> 
> 
>     $ qemu-img map /tmp/test.vmdk
>     Offset          Length          Mapped to       File
>     0               0x40000000      0               /tmp/test-flat.vmdk
> 
>     $ qemu-img map --output json /tmp/test.vmdk
>     [{"length": 1073741824, "start": 0, "zero": false, "offset": 0, "depth": 0,
>       "file": "/tmp/test-flat.vmdk", "data": true}
>     ]

Commented on patches 1, 4 and 11. The rest looks good to me.

Kevin

Re: [Qemu-devel] [PATCH v8 15/15] iotests: Add "qemu-img map" test for VMDK extents

[Max Reitz]

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

On 25.01.2016 03:44, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/059     | 10 ++++++++++
>  tests/qemu-iotests/059.out | 26 ++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
> index 0ded0c3..0332bbb 100755
> --- a/tests/qemu-iotests/059
> +++ b/tests/qemu-iotests/059
> @@ -133,6 +133,16 @@ $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io
>  $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
>  
>  echo
> +echo "=== Testing qemu-img map on extents ==="
> +for fmt in monolithicSparse twoGbMaxExtentSparse; do
> +    IMGOPTS="subformat=$fmt" _make_test_img 31G
> +    $QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io
> +    $QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io
> +    $QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io
> +    $QEMU_IMG map "$TEST_IMG" | _filter_testdir
> +done
> +
> +echo
>  echo "=== Testing afl image with a very large capacity ==="
>  _use_sample_img afl9.vmdk.bz2
>  _img_info
> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> index 9d506cb..5e041d7 100644
> --- a/tests/qemu-iotests/059.out
> +++ b/tests/qemu-iotests/059.out
> @@ -2050,6 +2050,7 @@ wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  wrote 512/512 bytes at offset 10240
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +qemu-img: Could not open '/home/fam/build/last/tests/qemu-iotests/scratch/t.vmdk': VMDK version 3 must be read only

I'd rather have the test fail than include this apparently wrong line
(according to
http://lists.nongnu.org/archive/html/qemu-block/2016-01/msg00710.html)
here, but I'll leave that up to you.

Reviewed-by: Max Reitz <mreitz@redhat.com>

>  
>  === Testing monolithicFlat with internally generated JSON file name ===
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 subformat=monolithicFlat
> @@ -2335,6 +2336,31 @@ e1000003f0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>  read 1024/1024 bytes at offset 966367641600
>  1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  
> +=== Testing qemu-img map on extents ===
> +Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=monolithicSparse
> +wrote 1024/1024 bytes at offset 65024
> +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 1024/1024 bytes at offset 2147483136
> +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 1024/1024 bytes at offset 5368709120
> +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Offset          Length          Mapped to       File
> +0               0x20000         0x3f0000        TEST_DIR/iotest-version3.vmdk
> +0x7fff0000      0x20000         0x410000        TEST_DIR/iotest-version3.vmdk
> +0x140000000     0x10000         0x430000        TEST_DIR/iotest-version3.vmdk
> +Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=twoGbMaxExtentSparse
> +wrote 1024/1024 bytes at offset 65024
> +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 1024/1024 bytes at offset 2147483136
> +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 1024/1024 bytes at offset 5368709120
> +1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Offset          Length          Mapped to       File
> +0               0x20000         0x50000         TEST_DIR/iotest-version3-s001.vmdk
> +0x7fff0000      0x10000         0x70000         TEST_DIR/iotest-version3-s001.vmdk
> +0x80000000      0x10000         0x50000         TEST_DIR/iotest-version3-s002.vmdk
> +0x140000000     0x10000         0x50000         TEST_DIR/iotest-version3-s003.vmdk
> +
>  === Testing afl image with a very large capacity ===
>  qemu-img: Can't get size of device 'image': File too large
>  *** done
> 



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

Re: [Qemu-devel] [PATCH v8 15/15] iotests: Add "qemu-img map" test for VMDK extents

[Max Reitz]

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

On 25.01.2016 15:11, Max Reitz wrote:
> On 25.01.2016 03:44, Fam Zheng wrote:
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  tests/qemu-iotests/059     | 10 ++++++++++
>>  tests/qemu-iotests/059.out | 26 ++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
>> index 0ded0c3..0332bbb 100755
>> --- a/tests/qemu-iotests/059
>> +++ b/tests/qemu-iotests/059
>> @@ -133,6 +133,16 @@ $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io
>>  $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
>>  
>>  echo
>> +echo "=== Testing qemu-img map on extents ==="
>> +for fmt in monolithicSparse twoGbMaxExtentSparse; do
>> +    IMGOPTS="subformat=$fmt" _make_test_img 31G
>> +    $QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io
>> +    $QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io
>> +    $QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io
>> +    $QEMU_IMG map "$TEST_IMG" | _filter_testdir
>> +done
>> +
>> +echo
>>  echo "=== Testing afl image with a very large capacity ==="
>>  _use_sample_img afl9.vmdk.bz2
>>  _img_info
>> diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
>> index 9d506cb..5e041d7 100644
>> --- a/tests/qemu-iotests/059.out
>> +++ b/tests/qemu-iotests/059.out
>> @@ -2050,6 +2050,7 @@ wrote 512/512 bytes at offset 0
>>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  wrote 512/512 bytes at offset 10240
>>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +qemu-img: Could not open '/home/fam/build/last/tests/qemu-iotests/scratch/t.vmdk': VMDK version 3 must be read only

...and I just noticed that my home directory is called /home/maxx and
not /home/fam. :-)

> I'd rather have the test fail than include this apparently wrong line
> (according to
> http://lists.nongnu.org/archive/html/qemu-block/2016-01/msg00710.html)
> here, but I'll leave that up to you.
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

So I'll have to revoke this R-b.

Max


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

Re: [Qemu-devel] [PATCH v8 01/15] block: Add "file" output parameter to block status query functions

[Fam Zheng]

On Mon, 01/25 14:04, Kevin Wolf wrote:
> Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> > The added parameter can be used to return the BDS pointer which the
> > valid offset is referring to. Its value should be ignored unless
> > BDRV_BLOCK_OFFSET_VALID in ret is set.
> > 
> > Until block drivers fill in the right value, let's clear it explicitly
> > right before calling .bdrv_get_block_status.
> > 
> > The "bs->file" condition in bdrv_co_get_block_status is kept now to keep iotest
> > case 102 passing, and will be fixed once all drivers return the right file
> > pointer.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/io.c                | 38 ++++++++++++++++++++++++++------------
> >  block/iscsi.c             |  6 ++++--
> >  block/mirror.c            |  3 ++-
> >  block/parallels.c         |  2 +-
> >  block/qcow.c              |  2 +-
> >  block/qcow2.c             |  2 +-
> >  block/qed.c               |  3 ++-
> >  block/raw-posix.c         |  3 ++-
> >  block/raw_bsd.c           |  3 ++-
> >  block/sheepdog.c          |  2 +-
> >  block/vdi.c               |  2 +-
> >  block/vmdk.c              |  2 +-
> >  block/vpc.c               |  2 +-
> >  block/vvfat.c             |  2 +-
> >  include/block/block.h     | 11 +++++++----
> >  include/block/block_int.h |  3 ++-
> >  qemu-img.c                | 13 +++++++++----
> >  17 files changed, 64 insertions(+), 35 deletions(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 5bb353a..0836991 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -664,6 +664,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
> >  int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
> >  {
> >      int64_t target_sectors, ret, nb_sectors, sector_num = 0;
> > +    BlockDriverState *file;
> >      int n;
> >  
> >      target_sectors = bdrv_nb_sectors(bs);
> > @@ -676,7 +677,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
> >          if (nb_sectors <= 0) {
> >              return 0;
> >          }
> > -        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
> > +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
> >          if (ret < 0) {
> >              error_report("error getting block status at sector %" PRId64 ": %s",
> >                           sector_num, strerror(-ret));
> > @@ -1466,6 +1467,7 @@ int bdrv_flush_all(void)
> >  typedef struct BdrvCoGetBlockStatusData {
> >      BlockDriverState *bs;
> >      BlockDriverState *base;
> > +    BlockDriverState **file;
> >      int64_t sector_num;
> >      int nb_sectors;
> >      int *pnum;
> > @@ -1490,7 +1492,8 @@ typedef struct BdrvCoGetBlockStatusData {
> >   */
> >  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >                                                       int64_t sector_num,
> > -                                                     int nb_sectors, int *pnum)
> > +                                                     int nb_sectors, int *pnum,
> > +                                                     BlockDriverState **file)
> 
> This function has a comment explaining all parameters. Except **file
> now.

Will add comment for @file too.

> 
> My impression at the moment is that it generally returns bs->file for
> format drivers and bs itself for protocol drivers if the sector is
> allocated in this layer.

Yes.

> 
> >  {
> >      int64_t total_sectors;
> >      int64_t n;
> > @@ -1520,16 +1523,19 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >          return ret;
> >      }
> >  
> > -    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
> > +    *file = NULL;
> > +    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
> > +                                            file);
> >      if (ret < 0) {
> >          *pnum = 0;
> > +        *file = NULL;
> 
> You specified that file is to be ignored unless BDRV_BLOCK_OFFSET_VALID
> is set. Why reset it here then? It's not reset in other error cases.

No particular reason, and it can be safely removed.

> 
> >          return ret;
> >      }
> >  
> >      if (ret & BDRV_BLOCK_RAW) {
> >          assert(ret & BDRV_BLOCK_OFFSET_VALID);
> >          return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
> > -                                     *pnum, pnum);
> > +                                     *pnum, pnum, file);
> >      }
> >  
> >      if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
> 
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 2ea5a4a..b8d29e1 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -2884,7 +2884,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
> >  }
> >  
> >  static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
> > -	int64_t sector_num, int nb_sectors, int* n)
> > +	int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
> >  {
> >      BDRVVVFATState* s = bs->opaque;
> >      *n = s->sector_count - sector_num;
> 
> This still returns NULL at the end of the series. Shouldn't it return bs
> like other protocol drivers do?

Yes, we need another patch for vvfat.

Thanks!

Fam

Re: [Qemu-devel] [PATCH v8 01/15] block: Add "file" output parameter to block status query functions

[Fam Zheng]

On Tue, 01/26 11:27, Fam Zheng wrote:
> On Mon, 01/25 14:04, Kevin Wolf wrote:
> > Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> > > diff --git a/block/vvfat.c b/block/vvfat.c
> > > index 2ea5a4a..b8d29e1 100644
> > > --- a/block/vvfat.c
> > > +++ b/block/vvfat.c
> > > @@ -2884,7 +2884,7 @@ static coroutine_fn int vvfat_co_write(BlockDriverState *bs, int64_t sector_num,
> > >  }
> > >  
> > >  static int64_t coroutine_fn vvfat_co_get_block_status(BlockDriverState *bs,
> > > -	int64_t sector_num, int nb_sectors, int* n)
> > > +	int64_t sector_num, int nb_sectors, int *n, BlockDriverState **file)
> > >  {
> > >      BDRVVVFATState* s = bs->opaque;
> > >      *n = s->sector_count - sector_num;
> > 
> > This still returns NULL at the end of the series. Shouldn't it return bs
> > like other protocol drivers do?
> 
> Yes, we need another patch for vvfat.

No, I now remember why vvfat didn't need a patch: it never sets the
BDRV_BLOCK_OFFSET_VALID bit.

Fam

Re: [Qemu-devel] [PATCH v8 04/15] raw: Assign bs to file in raw_co_get_block_status

[Fam Zheng]

On Mon, 01/25 14:17, Kevin Wolf wrote:
> Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> > diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> > index 9a8933b..fd355d5 100644
> > --- a/block/raw_bsd.c
> > +++ b/block/raw_bsd.c
> > @@ -119,6 +119,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
> >                                              BlockDriverState **file)
> >  {
> >      *pnum = nb_sectors;
> > +    *file = bs;
> 
> This should be bs->file->bs.
> 

Yes, will fix. I was confused about whether this is a format or a protocol.

Fam

Re: [Qemu-devel] [PATCH v8 11/15] vmdk: Return extent's file in bdrv_get_block_status

[Fam Zheng]

On Mon, 01/25 14:28, Kevin Wolf wrote:
> Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/vmdk.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index e1d3e27..f8f7fcf 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1274,6 +1274,7 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
> >                               0, 0);
> >      qemu_co_mutex_unlock(&s->lock);
> >  
> > +    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
> >      switch (ret) {
> >      case VMDK_ERROR:
> >          ret = -EIO;
> > @@ -1286,14 +1287,15 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
> >          break;
> >      case VMDK_OK:
> >          ret = BDRV_BLOCK_DATA;
> > -        if (extent->file == bs->file && !extent->compressed) {
> > -            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
> > +        if (!extent->compressed) {
> > +            ret |= BDRV_BLOCK_OFFSET_VALID;
> > +            ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
> > +                    & BDRV_BLOCK_OFFSET_MASK;
> >          }
> > -
> > +        *file = extent->file->bs;
> >          break;
> >      }
> >  
> > -    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
> >      n = extent->cluster_sectors - index_in_cluster;
> >      if (n > nb_sectors) {
> >          n = nb_sectors;
> 
> The commit message made me expect that this does exactly the same as the
> other patches, i.e. fill the file argument and nothing else.
> 
> Instead, this extends the functioniality to work on any kind of extents
> instead of just the the main VMDK file, and it changes the calculation
> of the offset (seems to be a hidden bug fix?)
> 
> This needs a non-empty commit message, and depending on what you're
> going to write there, possibly splitting the patch.

Will split it.

Fam

Re: [Qemu-devel] [PATCH v8 15/15] iotests: Add "qemu-img map" test for VMDK extents

[Fam Zheng]

On Mon, 01/25 15:11, Max Reitz wrote:
> On 25.01.2016 03:44, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  tests/qemu-iotests/059     | 10 ++++++++++
> >  tests/qemu-iotests/059.out | 26 ++++++++++++++++++++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
> > index 0ded0c3..0332bbb 100755
> > --- a/tests/qemu-iotests/059
> > +++ b/tests/qemu-iotests/059
> > @@ -133,6 +133,16 @@ $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io
> >  $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
> >  
> >  echo
> > +echo "=== Testing qemu-img map on extents ==="
> > +for fmt in monolithicSparse twoGbMaxExtentSparse; do
> > +    IMGOPTS="subformat=$fmt" _make_test_img 31G
> > +    $QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io
> > +    $QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io
> > +    $QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io
> > +    $QEMU_IMG map "$TEST_IMG" | _filter_testdir
> > +done
> > +
> > +echo
> >  echo "=== Testing afl image with a very large capacity ==="
> >  _use_sample_img afl9.vmdk.bz2
> >  _img_info
> > diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
> > index 9d506cb..5e041d7 100644
> > --- a/tests/qemu-iotests/059.out
> > +++ b/tests/qemu-iotests/059.out
> > @@ -2050,6 +2050,7 @@ wrote 512/512 bytes at offset 0
> >  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >  wrote 512/512 bytes at offset 10240
> >  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +qemu-img: Could not open '/home/fam/build/last/tests/qemu-iotests/scratch/t.vmdk': VMDK version 3 must be read only
> 
> I'd rather have the test fail than include this apparently wrong line
> (according to
> http://lists.nongnu.org/archive/html/qemu-block/2016-01/msg00710.html)
> here, but I'll leave that up to you.

It snuck in because of copy&paste when updating 059.out, I'll fix.

Fam