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

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

[Fam Zheng]

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: In 'map', use QDict to generate JSON output
  qemu-img: In "map" output, support external file name
  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 +-
 qemu-img.c                 | 49 ++++++++++++++++-------
 tests/qemu-iotests/059     | 10 +++++
 tests/qemu-iotests/059.out | 38 ++++++++++++++++++
 tests/qemu-iotests/122.out | 96 ++++++++++++++++++++++++++--------------------
 20 files changed, 213 insertions(+), 93 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.6 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 refering 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..67eaccb 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 (bs->file && *file == bs->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 4f79293..04f0e87 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

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

[Eric Blake]

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

On 11/23/2015 10:21 PM, Fam Zheng wrote:
> The added parameter can be used to return the BDS pointer which the
> valid offset is refering to. It's value should be ignored unless

s/refering/referring/

> 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>
> ---

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

Is there ever going to be a need to expose this through QMP, or is this
just used for the qemu-img map subcommand?

-- 
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 for-2.6 01/14] block: Add "file" output parameter to block status query functions

[Fam Zheng]

On Mon, 11/23 22:30, Eric Blake wrote:
> On 11/23/2015 10:21 PM, Fam Zheng wrote:
> > The added parameter can be used to return the BDS pointer which the
> > valid offset is refering to. It's value should be ignored unless
> 
> s/refering/referring/
> 
> > 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>
> > ---
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Is there ever going to be a need to expose this through QMP, or is this
> just used for the qemu-img map subcommand?
> 

I don't think QMP has exposure of information in block address level; for now
it is only used by qemu-img map, which has a "json" output format that can be
used by upper layer tools.

Thanks,
Fam

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

[Paolo Bonzini]

On 24/11/2015 06:21, Fam Zheng wrote:
> +    if (bs->file && *file == bs->file->bs &&

This check is unnecessary, just use "if (file)".

Paolo

>          (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);

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

[Fam Zheng]

On Tue, 11/24 12:47, Paolo Bonzini wrote:
> 
> 
> On 24/11/2015 06:21, Fam Zheng wrote:
> > +    if (bs->file && *file == bs->file->bs &&
> 
> This check is unnecessary, just use "if (file)".

"file" would be bs in the case of protocol, and this function will infinitely
recurse.

Fam

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

[Paolo Bonzini]

On 24/11/2015 13:50, Fam Zheng wrote:
> > > +    if (bs->file && *file == bs->file->bs &&
> > 
> > This check is unnecessary, just use "if (file)".
> 
> "file" would be bs in the case of protocol, and this function will infinitely
> recurse.

Oh, that's right!  But then I think we want to check that and allow
recursion to any *file, not just bs->file->bs.

Paolo

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

[Fam Zheng]

On Tue, 11/24 13:51, Paolo Bonzini wrote:
> 
> 
> On 24/11/2015 13:50, Fam Zheng wrote:
> > > > +    if (bs->file && *file == bs->file->bs &&
> > > 
> > > This check is unnecessary, just use "if (file)".
> > 
> > "file" would be bs in the case of protocol, and this function will infinitely
> > recurse.
> 
> Oh, that's right!  But then I think we want to check that and allow
> recursion to any *file, not just bs->file->bs.
> 

That makes sense, I'll make it to.

Fam

[Qemu-devel] [PATCH for-2.6 02/14] qcow: Assign bs->file->bs to file in qcow_co_get_block_status

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH for-2.6 02/14] qcow: Assign bs->file->bs to file in qcow_co_get_block_status

[Eric Blake]

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

On 11/23/2015 10:21 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/qcow.c | 1 +
>  1 file changed, 1 insertion(+)

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

> 
> 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;
>  }
>  
> 

-- 
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 --]

[Qemu-devel] [PATCH for-2.6 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

Re: [Qemu-devel] [PATCH for-2.6 03/14] qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status

[Eric Blake]

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

On 11/23/2015 10:22 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/qcow2.c | 1 +
>  1 file changed, 1 insertion(+)

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

> 
> 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) {
> 

-- 
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 --]

[Qemu-devel] [PATCH for-2.6 04/14] raw: Assign bs to file in raw_co_get_block_status

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH for-2.6 04/14] raw: Assign bs to file in raw_co_get_block_status

[Eric Blake]

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

On 11/23/2015 10:22 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 1 +
>  block/raw_bsd.c   | 1 +
>  2 files changed, 2 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 --]

[Qemu-devel] [PATCH for-2.6 05/14] iscsi: Assign bs to file in iscsi_co_get_block_status

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH for-2.6 05/14] iscsi: Assign bs to file in iscsi_co_get_block_status

[Eric Blake]

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

On 11/23/2015 10:22 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/iscsi.c | 3 +++
>  1 file changed, 3 insertions(+)

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

> 
> 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;
>  }
>  
> 

-- 
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 --]

[Qemu-devel] [PATCH for-2.6 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 04f0e87..d1406b1 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 for-2.6 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 for-2.6 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 for-2.6 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 for-2.6 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 for-2.6 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 for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Fam Zheng]

Switch json output generation from hand-written to QDict encoder, so
that the coming str field will be properly escaped.

Iotest 122 output is updated accordingly.

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

diff --git a/qemu-img.c b/qemu-img.c
index 7954242..97be910 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"
@@ -2146,6 +2148,9 @@ typedef struct MapEntry {
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
                            MapEntry *next)
 {
+    QDict *dict;
+    QString *str;
+
     switch (output_format) {
     case OFORMAT_HUMAN:
         if ((e->flags & BDRV_BLOCK_DATA) &&
@@ -2167,20 +2172,29 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         break;
     case OFORMAT_JSON:
-        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
-               " \"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->start == 0) {
+            printf("[");
+        } else {
+            printf(",");
+        }
+
+        dict = qdict_new();
+        qdict_put(dict, "start", qint_from_int(e->start));
+        qdict_put(dict, "length", qint_from_int(e->length));
+        qdict_put(dict, "depth", qint_from_int(e->depth));
+        qdict_put(dict, "zero", qbool_from_bool(e->flags & BDRV_BLOCK_ZERO));
+        qdict_put(dict, "data", qbool_from_bool(e->flags & BDRV_BLOCK_DATA));
         if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
-            printf(", \"offset\": %"PRId64"", e->offset);
+            qdict_put(dict, "offset", qint_from_int(e->offset));
         }
-        putchar('}');
+        str = qobject_to_json(QOBJECT(dict));
+        printf("%s\n", qstring_get_str(str));
 
         if (!next) {
             printf("]\n");
         }
+        QDECREF(str);
+        QDECREF(dict);
         break;
     }
 }
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 0068e96..3c119a8 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, "depth": 0, "data": true}
+,{"length": 60817408, "start": 6291456, "zero": true, "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, "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, "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, "depth": 0, "data": true}
+,{"length": 7168, "start": 1024, "zero": true, "depth": 0, "data": false}
+,{"length": 1024, "start": 8192, "zero": false, "offset": 9216, "depth": 0, "data": true}
+,{"length": 8192, "start": 9216, "zero": true, "depth": 0, "data": false}
+,{"length": 1024, "start": 17408, "zero": false, "offset": 10240, "depth": 0, "data": true}
+,{"length": 67090432, "start": 18432, "zero": true, "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, "depth": 0, "data": true}
+,{"length": 8192, "start": 9216, "zero": true, "depth": 0, "data": false}
+,{"length": 1024, "start": 17408, "zero": false, "offset": 17408, "depth": 0, "data": true}
+,{"length": 67090432, "start": 18432, "zero": true, "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

Re: [Qemu-devel] [PATCH for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Eric Blake]

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

On 11/23/2015 10:22 PM, Fam Zheng wrote:
> Switch json output generation from hand-written to QDict encoder, so
> that the coming str field will be properly escaped.
> 
> Iotest 122 output is updated accordingly.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img.c                 | 30 +++++++++++----
>  tests/qemu-iotests/122.out | 96 ++++++++++++++++++++++++++--------------------
>  2 files changed, 76 insertions(+), 50 deletions(-)
> 

>      case OFORMAT_JSON:
> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
> -               " \"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->start == 0) {
> +            printf("[");
> +        } else {
> +            printf(",");
> +        }
> +
> +        dict = qdict_new();
> +        qdict_put(dict, "start", qint_from_int(e->start));
> +        qdict_put(dict, "length", qint_from_int(e->length));
> +        qdict_put(dict, "depth", qint_from_int(e->depth));
> +        qdict_put(dict, "zero", qbool_from_bool(e->flags & BDRV_BLOCK_ZERO));
> +        qdict_put(dict, "data", qbool_from_bool(e->flags & BDRV_BLOCK_DATA));
>          if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
> -            printf(", \"offset\": %"PRId64"", e->offset);
> +            qdict_put(dict, "offset", qint_from_int(e->offset));
>          }
> -        putchar('}');
> +        str = qobject_to_json(QOBJECT(dict));
> +        printf("%s\n", qstring_get_str(str));

The conversion is sane...

> +++ 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}

...but the output shows that QDict output is tied to internal hashing
and therefore different than first-in-first-out creation order.
JSON-wise, it's still valid.  But are we guaranteed that our hashing is
stable?  If so, is that something an attacker could attempt to exploit
as a Denial of Service, by intentionally creating predictable
collisions?  [I'm guessing not, as the denial of service is mainly an
issue if a user can degrade performance of typical lookups from nominal
O(1) to O(n) by supplying LOTS of user-provided input, but the keys we
output in JSON are generally under our control via .json files and not
something where the user is dynamically creating lots of keys - but
still worth asking.]  And if it is not stable, then will our test break
when someone else's system hashes differently than yours?

-- 
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 for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Fam Zheng]

On Mon, 11/23 22:42, Eric Blake wrote:
> On 11/23/2015 10:22 PM, Fam Zheng wrote:
> > +++ 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}
> 
> ...but the output shows that QDict output is tied to internal hashing
> and therefore different than first-in-first-out creation order.
> JSON-wise, it's still valid.  But are we guaranteed that our hashing is
> stable?  If so, is that something an attacker could attempt to exploit
> as a Denial of Service, by intentionally creating predictable
> collisions?  [I'm guessing not, as the denial of service is mainly an
> issue if a user can degrade performance of typical lookups from nominal
> O(1) to O(n) by supplying LOTS of user-provided input, but the keys we
> output in JSON are generally under our control via .json files and not
> something where the user is dynamically creating lots of keys - but
> still worth asking.]

Interesting question.

I think creating a lot of colliding keys is possible, for example by creating a
huge set of options in the same level, or maybe just by providing a large
invalid 'json:' pseudo file name.

I wonder how QMP would handle this, because it also needs to parse json into
QObject.

> And if it is not stable, then will our test break
> when someone else's system hashes differently than yours?

We are using a deterministic hash function in qdict_put_obj, tdb_hash, so this
order is stable.

Fam

Re: [Qemu-devel] [PATCH for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Paolo Bonzini]

On 24/11/2015 06:22, Fam Zheng wrote:
>      case OFORMAT_JSON:
> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
> -               " \"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->start == 0) {
> +            printf("[");
> +        } else {
> +            printf(",");
> +        }
> +
> +        dict = qdict_new();
> +        qdict_put(dict, "start", qint_from_int(e->start));
> +        qdict_put(dict, "length", qint_from_int(e->length));
> +        qdict_put(dict, "depth", qint_from_int(e->depth));
> +        qdict_put(dict, "zero", qbool_from_bool(e->flags & BDRV_BLOCK_ZERO));
> +        qdict_put(dict, "data", qbool_from_bool(e->flags & BDRV_BLOCK_DATA));
>          if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
> -            printf(", \"offset\": %"PRId64"", e->offset);
> +            qdict_put(dict, "offset", qint_from_int(e->offset));
>          }
> -        putchar('}');
> +        str = qobject_to_json(QOBJECT(dict));
> +        printf("%s\n", qstring_get_str(str));

I think it's better if you use QAPI for this.  You can make MapEntry a
QAPI struct and generate the QObject through a QMP visitor.

The reason is that we could add JSON visitors that let us parse or
produce JSON without going through the expensive QObject creation.  Even
though that is far away, the least explicit QObject manipulation we
have, the better.

Paolo

Re: [Qemu-devel] [PATCH for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/11/2015 06:22, Fam Zheng wrote:
>>      case OFORMAT_JSON:
>> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
>> -               " \"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->start == 0) {
>> +            printf("[");
>> +        } else {
>> +            printf(",");
>> +        }
>> +
>> +        dict = qdict_new();
>> +        qdict_put(dict, "start", qint_from_int(e->start));
>> +        qdict_put(dict, "length", qint_from_int(e->length));
>> +        qdict_put(dict, "depth", qint_from_int(e->depth));
>> +        qdict_put(dict, "zero", qbool_from_bool(e->flags & BDRV_BLOCK_ZERO));
>> +        qdict_put(dict, "data", qbool_from_bool(e->flags & BDRV_BLOCK_DATA));
>>          if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
>> -            printf(", \"offset\": %"PRId64"", e->offset);
>> +            qdict_put(dict, "offset", qint_from_int(e->offset));
>>          }
>> -        putchar('}');
>> +        str = qobject_to_json(QOBJECT(dict));
>> +        printf("%s\n", qstring_get_str(str));
>
> I think it's better if you use QAPI for this.  You can make MapEntry a
> QAPI struct and generate the QObject through a QMP visitor.
>
> The reason is that we could add JSON visitors that let us parse or
> produce JSON without going through the expensive QObject creation.  Even
> though that is far away, the least explicit QObject manipulation we
> have, the better.

I concur.

Manual messing with QDict is of course fine when a higher-level
interface doesn't fit.  But here, we serialize a struct to JSON, and
that's something QAPI is meant to do.  Having to define a QAPI type may
be a bit of a bother, but once that's done, the "serialize this struct
to JSON" code becomes less tedious.  qemu-img.c already has a few
examples; search for qmp_output_visitor_new().

Re: [Qemu-devel] [PATCH for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Fam Zheng]

On Tue, 11/24 13:45, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 24/11/2015 06:22, Fam Zheng wrote:
> >>      case OFORMAT_JSON:
> >> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
> >> -               " \"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->start == 0) {
> >> +            printf("[");
> >> +        } else {
> >> +            printf(",");
> >> +        }
> >> +
> >> +        dict = qdict_new();
> >> +        qdict_put(dict, "start", qint_from_int(e->start));
> >> +        qdict_put(dict, "length", qint_from_int(e->length));
> >> +        qdict_put(dict, "depth", qint_from_int(e->depth));
> >> +        qdict_put(dict, "zero", qbool_from_bool(e->flags & BDRV_BLOCK_ZERO));
> >> +        qdict_put(dict, "data", qbool_from_bool(e->flags & BDRV_BLOCK_DATA));
> >>          if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
> >> -            printf(", \"offset\": %"PRId64"", e->offset);
> >> +            qdict_put(dict, "offset", qint_from_int(e->offset));
> >>          }
> >> -        putchar('}');
> >> +        str = qobject_to_json(QOBJECT(dict));
> >> +        printf("%s\n", qstring_get_str(str));
> >
> > I think it's better if you use QAPI for this.  You can make MapEntry a
> > QAPI struct and generate the QObject through a QMP visitor.
> >
> > The reason is that we could add JSON visitors that let us parse or
> > produce JSON without going through the expensive QObject creation.  Even
> > though that is far away, the least explicit QObject manipulation we
> > have, the better.
> 
> I concur.
> 
> Manual messing with QDict is of course fine when a higher-level
> interface doesn't fit.  But here, we serialize a struct to JSON, and
> that's something QAPI is meant to do.  Having to define a QAPI type may
> be a bit of a bother, but once that's done, the "serialize this struct
> to JSON" code becomes less tedious.  qemu-img.c already has a few
> examples; search for qmp_output_visitor_new().

Can we do streaming with QAPI? The size of "MapEntry array" can be huge.

Fam

Re: [Qemu-devel] [PATCH for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Fam Zheng]

On Tue, 11/24 21:25, Fam Zheng wrote:
> On Tue, 11/24 13:45, Markus Armbruster wrote:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> > > On 24/11/2015 06:22, Fam Zheng wrote:
> > >>      case OFORMAT_JSON:
> > >> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
> > >> -               " \"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->start == 0) {
> > >> +            printf("[");
> > >> +        } else {
> > >> +            printf(",");
> > >> +        }
> > >> +
> > >> +        dict = qdict_new();
> > >> +        qdict_put(dict, "start", qint_from_int(e->start));
> > >> +        qdict_put(dict, "length", qint_from_int(e->length));
> > >> +        qdict_put(dict, "depth", qint_from_int(e->depth));
> > >> +        qdict_put(dict, "zero", qbool_from_bool(e->flags & BDRV_BLOCK_ZERO));
> > >> +        qdict_put(dict, "data", qbool_from_bool(e->flags & BDRV_BLOCK_DATA));
> > >>          if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
> > >> -            printf(", \"offset\": %"PRId64"", e->offset);
> > >> +            qdict_put(dict, "offset", qint_from_int(e->offset));
> > >>          }
> > >> -        putchar('}');
> > >> +        str = qobject_to_json(QOBJECT(dict));
> > >> +        printf("%s\n", qstring_get_str(str));
> > >
> > > I think it's better if you use QAPI for this.  You can make MapEntry a
> > > QAPI struct and generate the QObject through a QMP visitor.
> > >
> > > The reason is that we could add JSON visitors that let us parse or
> > > produce JSON without going through the expensive QObject creation.  Even
> > > though that is far away, the least explicit QObject manipulation we
> > > have, the better.
> > 
> > I concur.
> > 
> > Manual messing with QDict is of course fine when a higher-level
> > interface doesn't fit.  But here, we serialize a struct to JSON, and
> > that's something QAPI is meant to do.  Having to define a QAPI type may
> > be a bit of a bother, but once that's done, the "serialize this struct
> > to JSON" code becomes less tedious.  qemu-img.c already has a few
> > examples; search for qmp_output_visitor_new().
> 
> Can we do streaming with QAPI? The size of "MapEntry array" can be huge.

Or you mean we don't have to serialize the list in one go, instead we can
serialize one MapEntry a time with QAPI like what we do now?

That sounds doable.

Fam

Re: [Qemu-devel] [PATCH for-2.6 12/14] qemu-img: In 'map', use QDict to generate JSON output

[Paolo Bonzini]

On 24/11/2015 14:30, Fam Zheng wrote:
> Or you mean we don't have to serialize the list in one go, instead we can
> serialize one MapEntry a time with QAPI like what we do now?

Yes, serializing the whole list in one go needs a visitor that can do
streaming.  We can write the array manually, and use QAPI to write each
dictionary inside the array.

Paolo

[Qemu-devel] [PATCH for-2.6 13/14] qemu-img: In "map" output, support external file name

[Fam Zheng]

The new "file" output parameter of bdrv_get_block_status tells which
file the valid offset is referring to, we can use the information and
output the filename.

The iotest 122 reference output is updated accordingly.

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

diff --git a/qemu-img.c b/qemu-img.c
index 97be910..666af66 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2142,7 +2142,7 @@ typedef struct MapEntry {
     int64_t start;
     int64_t length;
     int64_t offset;
-    BlockDriverState *bs;
+    BlockDriverState *file;
 } MapEntry;
 
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
@@ -2155,12 +2155,13 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
     case OFORMAT_HUMAN:
         if ((e->flags & BDRV_BLOCK_DATA) &&
             !(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
-            error_report("File contains external, encrypted or compressed clusters.");
+            error_report("File contains encrypted or compressed clusters.");
             exit(1);
         }
         if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
             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->file ? e->file->filename : "");
         }
         /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
          * Modify the flags here to allow more coalescing.
@@ -2186,6 +2187,9 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
         qdict_put(dict, "data", qbool_from_bool(e->flags & BDRV_BLOCK_DATA));
         if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
             qdict_put(dict, "offset", qint_from_int(e->offset));
+            if (e->file) {
+                qdict_put(dict, "file", qstring_from_str(e->file->filename));
+            }
         }
         str = qobject_to_json(QOBJECT(dict));
         printf("%s\n", qstring_get_str(str));
@@ -2236,7 +2240,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->file = file;
     return 0;
 }
 
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 3c119a8..cb249ce 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -114,7 +114,7 @@ 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)
-[{"length": 6291456, "start": 0, "zero": false, "offset": 327680, "depth": 0, "data": true}
+[{"length": 6291456, "start": 0, "zero": false, "offset": 327680, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ,{"length": 60817408, "start": 6291456, "zero": true, "depth": 0, "data": false}
 ]
 
@@ -140,7 +140,7 @@ 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)
-[{"length": 67108864, "start": 0, "zero": false, "offset": 327680, "depth": 0, "data": true}
+[{"length": 67108864, "start": 0, "zero": false, "offset": 327680, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ]
 
 convert -c -S 0 with source backing file:
@@ -160,7 +160,7 @@ 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)
-[{"length": 67108864, "start": 0, "zero": false, "offset": 327680, "depth": 0, "data": true}
+[{"length": 67108864, "start": 0, "zero": false, "offset": 327680, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ]
 
 convert -c -S 0 -B ...
@@ -186,11 +186,11 @@ 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
-[{"length": 1024, "start": 0, "zero": false, "offset": 8192, "depth": 0, "data": true}
+[{"length": 1024, "start": 0, "zero": false, "offset": 8192, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ,{"length": 7168, "start": 1024, "zero": true, "depth": 0, "data": false}
-,{"length": 1024, "start": 8192, "zero": false, "offset": 9216, "depth": 0, "data": true}
+,{"length": 1024, "start": 8192, "zero": false, "offset": 9216, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ,{"length": 8192, "start": 9216, "zero": true, "depth": 0, "data": false}
-,{"length": 1024, "start": 17408, "zero": false, "offset": 10240, "depth": 0, "data": true}
+,{"length": 1024, "start": 17408, "zero": false, "offset": 10240, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ,{"length": 67090432, "start": 18432, "zero": true, "depth": 0, "data": false}
 ]
 
@@ -204,9 +204,9 @@ convert -c -S 4k
 ]
 
 convert -S 8k
-[{"length": 9216, "start": 0, "zero": false, "offset": 8192, "depth": 0, "data": true}
+[{"length": 9216, "start": 0, "zero": false, "offset": 8192, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ,{"length": 8192, "start": 9216, "zero": true, "depth": 0, "data": false}
-,{"length": 1024, "start": 17408, "zero": false, "offset": 17408, "depth": 0, "data": true}
+,{"length": 1024, "start": 17408, "zero": false, "offset": 17408, "depth": 0, "file": "TEST_DIR/t.IMGFMT.orig", "data": true}
 ,{"length": 67090432, "start": 18432, "zero": true, "depth": 0, "data": false}
 ]
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.6 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