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

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

[Fam Zheng]

v2: Add Eric's rev-by in patches 2, 4, 5.
    01: Refering -> referring in commit message. [Eric]
        Recurse to "file" for sensible "zero" flag. [Paolo]
    12: New. Make MapEntry a QAPI struct. [Paolo, Markus]

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 (14):
  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
  qemu-img: Make MapEntry a QAPI struct
  qemu-img: Use QAPI visitor to generate JSON
  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               | 13 +++----
 block/vpc.c                |  4 +-
 block/vvfat.c              |  2 +-
 include/block/block.h      |  6 ++-
 include/block/block_int.h  |  3 +-
 qapi/block-core.json       | 28 ++++++++++++++
 qemu-img.c                 | 80 +++++++++++++++++++++-----------------
 tests/qemu-iotests/059     | 10 +++++
 tests/qemu-iotests/059.out | 38 ++++++++++++++++++
 tests/qemu-iotests/122.out | 96 ++++++++++++++++++++++++++--------------------
 21 files changed, 251 insertions(+), 114 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 01/14] 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. It's 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.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c                | 42 ++++++++++++++++++++++++++++--------------
 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     |  6 ++++--
 include/block/block_int.h |  3 ++-
 qemu-img.c                |  7 +++++--
 17 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/block/io.c b/block/io.c
index adc1eab..71930ed 100644
--- a/block/io.c
+++ b/block/io.c
@@ -656,6 +656,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);
@@ -668,7 +669,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));
@@ -1455,6 +1456,7 @@ int bdrv_flush_all(void)
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
+    BlockDriverState **file;
     int64_t sector_num;
     int nb_sectors;
     int *pnum;
@@ -1479,7 +1481,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;
@@ -1509,16 +1512,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)) {
@@ -1535,13 +1541,14 @@ 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,
-                                        *pnum, &file_pnum);
+        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
              * is useful but not necessary.
@@ -1566,14 +1573,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;
         }
@@ -1592,7 +1600,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;
 }
 
@@ -1604,12 +1613,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,
@@ -1633,16 +1644,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 bd1f1bf..2d1e230 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 52c9abf..c7a26b7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -166,6 +166,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     MirrorOp *op;
     int pnum;
     int64_t ret;
+    BlockDriverState *file;
 
     s->sector_num = hbitmap_iter_next(&s->hbi);
     if (s->sector_num < 0) {
@@ -302,7 +303,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 f689fde..d1146f1 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -260,7 +260,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 635085e..558f443 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -488,7 +488,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 88f56c8..836888c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1283,7 +1283,7 @@ static void qcow2_reopen_abort(BDRVReopenState *state)
 }
 
 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 9b88895..a6bbd8b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -724,7 +724,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 aec9ec6..2cd7d68 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1830,7 +1830,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 915d6fd..7d23c08 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -114,7 +114,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 d80e4ed..0f6789e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2709,7 +2709,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 17f435f..2199fd3 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -526,7 +526,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 6f819e4..f5a56fd 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1248,7 +1248,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 299d373..912f5d0 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -578,7 +578,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 b184eca..b6b410a 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 73edb1a..60e7ae6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -383,11 +383,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 4012e36..d890abb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -165,7 +165,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 033011c..7954242 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1259,9 +1259,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;
         }
@@ -2189,6 +2190,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
@@ -2197,7 +2199,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 v2 02/14] qcow: Assign bs->file->bs to file in qcow_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@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 558f443..b59383f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -509,6 +509,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 v2 03/14] qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status

[Fam Zheng]

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 836888c..7634c42 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1302,6 +1302,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 v2 04/14] raw: Assign bs to file in raw_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@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 2cd7d68..9988aa4 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1873,6 +1873,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 7d23c08..2d4c896 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -118,6 +118,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 v2 05/14] iscsi: Assign bs to file in iscsi_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake <eblake@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 2d1e230..8c7f1b3 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 v2 06/14] parallels: Assign bs->file->bs to file in parallels_co_get_block_status

[Fam Zheng]

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 d1146f1..6552f32 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -273,6 +273,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 v2 07/14] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status

[Fam Zheng]

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 a6bbd8b..03af9c1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -692,6 +692,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)
@@ -703,6 +704,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;
@@ -734,6 +736,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 v2 08/14] sheepdog: Assign bs to file in sd_co_get_block_status

[Fam Zheng]

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 0f6789e..d5e7ff8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2740,6 +2740,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 v2 09/14] vdi: Assign bs->file->bs to file in vdi_co_get_block_status

[Fam Zheng]

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 2199fd3..6b1a57b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -550,6 +550,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 v2 10/14] vpc: Assign bs->file->bs to file in vpc_co_get_block_status

[Fam Zheng]

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 912f5d0..412ff41 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -588,6 +588,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);
     }
@@ -609,6 +610,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 v2 11/14] vmdk: Return extent's file in bdrv_get_block_status

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index f5a56fd..b60a5af 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1265,6 +1265,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;
@@ -1276,15 +1277,13 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
         ret = BDRV_BLOCK_ZERO;
         break;
     case VMDK_OK:
-        ret = BDRV_BLOCK_DATA;
-        if (extent->file == bs->file && !extent->compressed) {
-            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
-        }
-
+        ret = BDRV_BLOCK_DATA | 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 v2 12/14] 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.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi/block-core.json | 28 ++++++++++++++++++++++++++++
 qemu-img.c           | 48 ++++++++++++++++++++++--------------------------
 2 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index a07b13f..db62bda 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -186,6 +186,34 @@
            '*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 7954242..0ccb396 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2133,47 +2133,36 @@ 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->offset,
+                   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 = true;
         }
         break;
     case OFORMAT_JSON:
-        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
+        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
+               " \"depth\": %ld,"
                " \"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('}');
@@ -2219,10 +2208,15 @@ 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 = bs;
+    if (e->has_offset) {
+        e->has_filename = true;
+        e->filename = bs->filename;
+    }
     return 0;
 }
 
@@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv)
             goto out;
         }
 
-        if (curr.length != 0 && curr.flags == next.flags &&
+        if (curr.length != 0 && curr.zero == next.zero &&
+            curr.data == next.data &&
             curr.depth == next.depth &&
-            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
+            !strcmp(curr.filename, next.filename) &&
+            (!curr.has_offset ||
              curr.offset + curr.length == next.offset)) {
             curr.length += next.length;
             continue;
-- 
2.4.3

[Qemu-devel] [PATCH v2 13/14] qemu-img: Use QAPI visitor to generate JSON

[Fam Zheng]

A visible improvement is that "filename" is now included in the output
if it's valid.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c                 | 39 ++++++++++++-------
 tests/qemu-iotests/122.out | 96 ++++++++++++++++++++++++++--------------------
 2 files changed, 79 insertions(+), 56 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 0ccb396..7b00c7c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -25,6 +25,8 @@
 #include "qapi/qmp-output-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
 #include "qemu-common.h"
 #include "qemu/option.h"
 #include "qemu/error-report.h"
@@ -2136,10 +2138,14 @@ static int img_info(int argc, char **argv)
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
                            MapEntry *next)
 {
+    QString *str;
+    QObject *obj;
+    QmpOutputVisitor *ov;
+
     switch (output_format) {
     case OFORMAT_HUMAN:
         if (e->data && !e->has_offset) {
-            error_report("File contains external, encrypted or compressed clusters.");
+            error_report("File contains encrypted or compressed clusters.");
             exit(1);
         }
         if (e->data && !e->zero) {
@@ -2155,21 +2161,26 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         break;
     case OFORMAT_JSON:
-        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
-               " \"depth\": %ld,"
-               " \"zero\": %s, \"data\": %s",
-               (e->start == 0 ? "[" : ",\n"),
-               e->start, e->length, e->depth,
-               e->zero ? "true" : "false",
-               e->data ? "true" : "false");
-        if (e->has_offset) {
-            printf(", \"offset\": %"PRId64"", e->offset);
-        }
-        putchar('}');
+        ov = qmp_output_visitor_new();
+        visit_type_MapEntry(qmp_output_get_visitor(ov),
+                            &e, NULL, &error_abort);
+        obj = qmp_output_get_qobject(ov);
+        str = qobject_to_json(obj);
+        assert(str != NULL);
 
+        if (e->start == 0) {
+            printf("[");
+        } else {
+            printf(",");
+        }
+        printf("%s\n", qstring_get_str(str));
         if (!next) {
             printf("]\n");
         }
+
+        qobject_decref(obj);
+        qmp_output_visitor_cleanup(ov);
+        QDECREF(str);
         break;
     }
 }
@@ -2213,9 +2224,9 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
     e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
     e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
     e->depth = depth;
-    if (e->has_offset) {
+    if (file && e->has_offset) {
         e->has_filename = true;
-        e->filename = bs->filename;
+        e->filename = file->filename;
     }
     return 0;
 }
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 0068e96..760759e 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -49,12 +49,13 @@ read 65536/65536 bytes at offset 4194304
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 8388608
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 4128768, "depth": 0, "zero": true, "data": false},
-{ "start": 4194304, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 4259840, "length": 4128768, "depth": 0, "zero": true, "data": false},
-{ "start": 8388608, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 8454144, "length": 4128768, "depth": 0, "zero": true, "data": false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 4194304, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 4259840, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 8388608, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 8454144, "zero": true, "depth": 0, "data": false}
+]
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 4194304
@@ -76,12 +77,13 @@ wrote 1024/1024 bytes at offset 1046528
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 65536, "depth": 0, "zero": true, "data": false},
-{ "start": 131072, "length": 196608, "depth": 0, "zero": false, "data": true},
-{ "start": 327680, "length": 655360, "depth": 0, "zero": true, "data": false},
-{ "start": 983040, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 1048576, "length": 1046528, "depth": 0, "zero": true, "data": false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 65536, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 196608, "start": 131072, "zero": false, "depth": 0, "data": true}
+,{"length": 655360, "start": 327680, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 983040, "zero": false, "depth": 0, "data": true}
+,{"length": 1046528, "start": 1048576, "zero": true, "depth": 0, "data": false}
+]
 read 16384/16384 bytes at offset 0
 16 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 16384/16384 bytes at offset 16384
@@ -112,16 +114,18 @@ read 3145728/3145728 bytes at offset 0
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 63963136/63963136 bytes at offset 3145728
 61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": 327680},
-{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
+[{"length": 6291456, "start": 0, "zero": false, "offset": 327680, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+,{"length": 60817408, "start": 6291456, "zero": true, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": false}
+]
 
 convert -c -S 0:
 read 3145728/3145728 bytes at offset 0
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 63963136/63963136 bytes at offset 3145728
 61 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 6291456, "depth": 0, "zero": false, "data": true},
-{ "start": 6291456, "length": 60817408, "depth": 0, "zero": true, "data": false}]
+[{"length": 6291456, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 60817408, "start": 6291456, "zero": true, "depth": 0, "data": false}
+]
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
 wrote 33554432/33554432 bytes at offset 0
 32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -136,7 +140,8 @@ read 30408704/30408704 bytes at offset 3145728
 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 33554432/33554432 bytes at offset 33554432
 32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
+[{"length": 67108864, "start": 0, "zero": false, "offset": 327680, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+]
 
 convert -c -S 0 with source backing file:
 read 3145728/3145728 bytes at offset 0
@@ -145,7 +150,8 @@ read 30408704/30408704 bytes at offset 3145728
 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 33554432/33554432 bytes at offset 33554432
 32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true}]
+[{"length": 67108864, "start": 0, "zero": false, "depth": 0, "data": true}
+]
 
 convert -S 0 -B ...
 read 3145728/3145728 bytes at offset 0
@@ -154,7 +160,8 @@ read 30408704/30408704 bytes at offset 3145728
 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 33554432/33554432 bytes at offset 33554432
 32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true, "offset": 327680}]
+[{"length": 67108864, "start": 0, "zero": false, "offset": 327680, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+]
 
 convert -c -S 0 -B ...
 read 3145728/3145728 bytes at offset 0
@@ -163,7 +170,8 @@ read 30408704/30408704 bytes at offset 3145728
 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 33554432/33554432 bytes at offset 33554432
 32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 67108864, "depth": 0, "zero": false, "data": true}]
+[{"length": 67108864, "start": 0, "zero": false, "depth": 0, "data": true}
+]
 
 === Non-zero -S ===
 
@@ -178,32 +186,36 @@ wrote 1024/1024 bytes at offset 17408
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 convert -S 4k
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": 8192},
-{ "start": 1024, "length": 7168, "depth": 0, "zero": true, "data": false},
-{ "start": 8192, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": 9216},
-{ "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
-{ "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": 10240},
-{ "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+[{"length": 1024, "start": 0, "zero": false, "offset": 8192, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+,{"length": 7168, "start": 1024, "zero": true, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": false}
+,{"length": 1024, "start": 8192, "zero": false, "offset": 9216, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+,{"length": 8192, "start": 9216, "zero": true, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": false}
+,{"length": 1024, "start": 17408, "zero": false, "offset": 10240, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+,{"length": 67090432, "start": 18432, "zero": true, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": false}
+]
 
 convert -c -S 4k
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true},
-{ "start": 1024, "length": 7168, "depth": 0, "zero": true, "data": false},
-{ "start": 8192, "length": 1024, "depth": 0, "zero": false, "data": true},
-{ "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
-{ "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true},
-{ "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+[{"length": 1024, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 7168, "start": 1024, "zero": true, "depth": 0, "data": false}
+,{"length": 1024, "start": 8192, "zero": false, "depth": 0, "data": true}
+,{"length": 8192, "start": 9216, "zero": true, "depth": 0, "data": false}
+,{"length": 1024, "start": 17408, "zero": false, "depth": 0, "data": true}
+,{"length": 67090432, "start": 18432, "zero": true, "depth": 0, "data": false}
+]
 
 convert -S 8k
-[{ "start": 0, "length": 9216, "depth": 0, "zero": false, "data": true, "offset": 8192},
-{ "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
-{ "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": 17408},
-{ "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+[{"length": 9216, "start": 0, "zero": false, "offset": 8192, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+,{"length": 8192, "start": 9216, "zero": true, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": false}
+,{"length": 1024, "start": 17408, "zero": false, "offset": 17408, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": true}
+,{"length": 67090432, "start": 18432, "zero": true, "filename": "TEST_DIR/t.IMGFMT.orig", "depth": 0, "data": false}
+]
 
 convert -c -S 8k
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true},
-{ "start": 1024, "length": 7168, "depth": 0, "zero": true, "data": false},
-{ "start": 8192, "length": 1024, "depth": 0, "zero": false, "data": true},
-{ "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
-{ "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true},
-{ "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+[{"length": 1024, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 7168, "start": 1024, "zero": true, "depth": 0, "data": false}
+,{"length": 1024, "start": 8192, "zero": false, "depth": 0, "data": true}
+,{"length": 8192, "start": 9216, "zero": true, "depth": 0, "data": false}
+,{"length": 1024, "start": 17408, "zero": false, "depth": 0, "data": true}
+,{"length": 67090432, "start": 18432, "zero": true, "depth": 0, "data": false}
+]
 *** done
-- 
2.4.3

[Qemu-devel] [PATCH v2 14/14] 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 | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 0ded0c3..261d8b0 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 twoGbMaxExtentSparse twoGbMaxExtentFlat; 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 00057fe..54eb530 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2337,6 +2337,44 @@ 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=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
+Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 subformat=twoGbMaxExtentFlat
+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               0x80000000      0               TEST_DIR/iotest-version3-f001.vmdk
+0x80000000      0x80000000      0               TEST_DIR/iotest-version3-f002.vmdk
+0x100000000     0x80000000      0               TEST_DIR/iotest-version3-f003.vmdk
+0x180000000     0x80000000      0               TEST_DIR/iotest-version3-f004.vmdk
+0x200000000     0x80000000      0               TEST_DIR/iotest-version3-f005.vmdk
+0x280000000     0x80000000      0               TEST_DIR/iotest-version3-f006.vmdk
+0x300000000     0x80000000      0               TEST_DIR/iotest-version3-f007.vmdk
+0x380000000     0x80000000      0               TEST_DIR/iotest-version3-f008.vmdk
+0x400000000     0x80000000      0               TEST_DIR/iotest-version3-f009.vmdk
+0x480000000     0x80000000      0               TEST_DIR/iotest-version3-f010.vmdk
+0x500000000     0x80000000      0               TEST_DIR/iotest-version3-f011.vmdk
+0x580000000     0x80000000      0               TEST_DIR/iotest-version3-f012.vmdk
+0x600000000     0x80000000      0               TEST_DIR/iotest-version3-f013.vmdk
+0x680000000     0x80000000      0               TEST_DIR/iotest-version3-f014.vmdk
+0x700000000     0x80000000      0               TEST_DIR/iotest-version3-f015.vmdk
+0x780000000     0x40000000      0               TEST_DIR/iotest-version3-f016.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 v2 12/14] qemu-img: Make MapEntry a QAPI struct

[Paolo Bonzini]

On 25/11/2015 08:39, Fam Zheng wrote:
>           */
> -        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 = true;

		before		after
0		ZERO		ZERO
DATA		DATA		DATA
DATA|ZERO	ZERO		DATA|ZERO
ZERO		ZERO		ZERO

This would not coalesce 0 with DATA|ZERO.

I think you need to do exactly as in the older code: test (!next->data
|| next->zero), and clear next->data.

Paolo

Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> The "flags" bit mask is expanded to two booleans, "data" and "zero";
> "bs" is replaced with "filename" string.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qapi/block-core.json | 28 ++++++++++++++++++++++++++++
>  qemu-img.c           | 48 ++++++++++++++++++++++--------------------------
>  2 files changed, 50 insertions(+), 26 deletions(-)
> 

> +##
> +
> +{ 'struct': 'MapEntry',

Blank line is not typical here.

> +  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> +           'zero': 'bool', 'depth': 'int', '*offset': 'int',
> +           '*filename': 'str' } }

Struct looks fine.

> 
> -        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->offset,
> +                   e->has_filename ? e->filename : "");
>          }

This blindly prints e->offset, even though...[1]

>      case OFORMAT_JSON:
> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
> +        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> +               " \"depth\": %ld,"

%ld is wrong; it needs to be PRId64.

>                 " \"zero\": %s, \"data\": %s",

Worth joining the two short lines into one?

> @@ -2219,10 +2208,15 @@ 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);

[1]... offset might be garbage if has_offset is false.

>      e->depth = depth;
> -    e->bs = bs;
> +    if (e->has_offset) {
> +        e->has_filename = true;
> +        e->filename = bs->filename;
> +    }
>      return 0;
>  }

Are we guaranteed that bs->filename is non-NULL when offset is set?  Or
does this need to be if (e->has_offset && bs->filename)?

>  
> @@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv)
>              goto out;
>          }
>  
> -        if (curr.length != 0 && curr.flags == next.flags &&
> +        if (curr.length != 0 && curr.zero == next.zero &&
> +            curr.data == next.data &&
>              curr.depth == next.depth &&
> -            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
> +            !strcmp(curr.filename, next.filename) &&

Is this strcmp valid even when !has_filename?

> +            (!curr.has_offset ||
>               curr.offset + curr.length == next.offset)) {
>              curr.length += next.length;
>              continue;
> 

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


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

Re: [Qemu-devel] [PATCH v2 13/14] qemu-img: Use QAPI visitor to generate JSON

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> A visible improvement is that "filename" is now included in the output
> if it's valid.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img.c                 | 39 ++++++++++++-------
>  tests/qemu-iotests/122.out | 96 ++++++++++++++++++++++++++--------------------
>  2 files changed, 79 insertions(+), 56 deletions(-)
> 

> @@ -2155,21 +2161,26 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
>          }
>          break;
>      case OFORMAT_JSON:
> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> -               " \"depth\": %ld,"
> -               " \"zero\": %s, \"data\": %s",
> -               (e->start == 0 ? "[" : ",\n"),
> -               e->start, e->length, e->depth,
> -               e->zero ? "true" : "false",
> -               e->data ? "true" : "false");
> -        if (e->has_offset) {
> -            printf(", \"offset\": %"PRId64"", e->offset);
> -        }
> -        putchar('}');
> +        ov = qmp_output_visitor_new();
> +        visit_type_MapEntry(qmp_output_get_visitor(ov),
> +                            &e, NULL, &error_abort);
> +        obj = qmp_output_get_qobject(ov);
> +        str = qobject_to_json(obj);

It would be nice to someday add a visitor that goes directly to json,
instead of through an intermediate QObject. But that's not for this
series; your conversion here is sane.

> @@ -2213,9 +2224,9 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
>      e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
>      e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
>      e->depth = depth;
> -    if (e->has_offset) {
> +    if (file && e->has_offset) {
>          e->has_filename = true;
> -        e->filename = bs->filename;
> +        e->filename = file->filename;

Does this hunk belong in a different patch?

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

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

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  tests/qemu-iotests/059     | 10 ++++++++++
>  tests/qemu-iotests/059.out | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v2 06/14] parallels: Assign bs->file->bs to file in parallels_co_get_block_status

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/parallels.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/parallels.c b/block/parallels.c
> index d1146f1..6552f32 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -273,6 +273,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;
>  }
> 

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


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

Re: [Qemu-devel] [PATCH v2 07/14] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/qed.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

[Fam Zheng]

On Wed, 11/25 08:36, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > The "flags" bit mask is expanded to two booleans, "data" and "zero";
> > "bs" is replaced with "filename" string.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  qapi/block-core.json | 28 ++++++++++++++++++++++++++++
> >  qemu-img.c           | 48 ++++++++++++++++++++++--------------------------
> >  2 files changed, 50 insertions(+), 26 deletions(-)
> > 
> 
> > +##
> > +
> > +{ 'struct': 'MapEntry',
> 
> Blank line is not typical here.

This is copied from ImageInfo above. Removing.

> 
> > +  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> > +           'zero': 'bool', 'depth': 'int', '*offset': 'int',
> > +           '*filename': 'str' } }
> 
> Struct looks fine.
> 
> > 
> > -        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->offset,
> > +                   e->has_filename ? e->filename : "");
> >          }
> 
> This blindly prints e->offset, even though...[1]

Will add check.

> 
> >      case OFORMAT_JSON:
> > -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
> > +        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> > +               " \"depth\": %ld,"
> 
> %ld is wrong; it needs to be PRId64.

Yes.

> 
> >                 " \"zero\": %s, \"data\": %s",
> 
> Worth joining the two short lines into one?

OK.

> 
> > @@ -2219,10 +2208,15 @@ 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);
> 
> [1]... offset might be garbage if has_offset is false.
> 
> >      e->depth = depth;
> > -    e->bs = bs;
> > +    if (e->has_offset) {
> > +        e->has_filename = true;
> > +        e->filename = bs->filename;
> > +    }
> >      return 0;
> >  }
> 
> Are we guaranteed that bs->filename is non-NULL when offset is set?  Or
> does this need to be if (e->has_offset && bs->filename)?

It's an array field:

    struct BlockDriverState {
        ...
        char filename[PATH_MAX];

So I think this is OK.

> 
> >  
> > @@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv)
> >              goto out;
> >          }
> >  
> > -        if (curr.length != 0 && curr.flags == next.flags &&
> > +        if (curr.length != 0 && curr.zero == next.zero &&
> > +            curr.data == next.data &&
> >              curr.depth == next.depth &&
> > -            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
> > +            !strcmp(curr.filename, next.filename) &&
> 
> Is this strcmp valid even when !has_filename?

No, will check it.

Thanks for reviewing!

Fam

> 
> > +            (!curr.has_offset ||
> >               curr.offset + curr.length == next.offset)) {
> >              curr.length += next.length;
> >              continue;
> > 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v2 13/14] qemu-img: Use QAPI visitor to generate JSON

[Fam Zheng]

On Wed, 11/25 08:42, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > A visible improvement is that "filename" is now included in the output
> > if it's valid.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  qemu-img.c                 | 39 ++++++++++++-------
> >  tests/qemu-iotests/122.out | 96 ++++++++++++++++++++++++++--------------------
> >  2 files changed, 79 insertions(+), 56 deletions(-)
> > 
> 
> > @@ -2155,21 +2161,26 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
> >          }
> >          break;
> >      case OFORMAT_JSON:
> > -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> > -               " \"depth\": %ld,"
> > -               " \"zero\": %s, \"data\": %s",
> > -               (e->start == 0 ? "[" : ",\n"),
> > -               e->start, e->length, e->depth,
> > -               e->zero ? "true" : "false",
> > -               e->data ? "true" : "false");
> > -        if (e->has_offset) {
> > -            printf(", \"offset\": %"PRId64"", e->offset);
> > -        }
> > -        putchar('}');
> > +        ov = qmp_output_visitor_new();
> > +        visit_type_MapEntry(qmp_output_get_visitor(ov),
> > +                            &e, NULL, &error_abort);
> > +        obj = qmp_output_get_qobject(ov);
> > +        str = qobject_to_json(obj);
> 
> It would be nice to someday add a visitor that goes directly to json,
> instead of through an intermediate QObject. But that's not for this
> series; your conversion here is sane.
> 
> > @@ -2213,9 +2224,9 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
> >      e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
> >      e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
> >      e->depth = depth;
> > -    if (e->has_offset) {
> > +    if (file && e->has_offset) {
> >          e->has_filename = true;
> > -        e->filename = bs->filename;
> > +        e->filename = file->filename;
> 
> Does this hunk belong in a different patch?
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Yes, I can split this into a separate patch.

Fam

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

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> The added parameter can be used to return the BDS pointer which the
> valid offset is referring to. It's value should be ignored unless

s/It's/Its/  (remember, "It's" is valid only if "It is" also works in
the same place)

> 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.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---

With that fix,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH v2 08/14] sheepdog: Assign bs to file in sd_co_get_block_status

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/sheepdog.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0f6789e..d5e7ff8 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2740,6 +2740,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;
>  }
>  
> 

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


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

Re: [Qemu-devel] [PATCH v2 09/14] vdi: Assign bs->file->bs to file in vdi_co_get_block_status

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vdi.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 2199fd3..6b1a57b 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -550,6 +550,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;
>  }
>  
> 

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


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

Re: [Qemu-devel] [PATCH v2 10/14] vpc: Assign bs->file->bs to file in vpc_co_get_block_status

[Eric Blake]

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

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vpc.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 912f5d0..412ff41 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -588,6 +588,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);
>      }
> @@ -609,6 +610,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) {
> 

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


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

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

[Fam Zheng]

On Wed, 11/25 22:03, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > The added parameter can be used to return the BDS pointer which the
> > valid offset is referring to. It's value should be ignored unless
> 
> s/It's/Its/  (remember, "It's" is valid only if "It is" also works in
> the same place)

Right, it's just easy to typo. Thanks :)

Fam

> 
> > 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.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> 
> With that fix,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>