[Qemu-devel] [PATCH v4 0/4] more blkdebug tweaks

[Qemu-devel] [PATCH v4 0/4] more blkdebug tweaks

[Eric Blake]

I found a crasher and some odd behavior while rebasing my
bdrv_get_block_status series, so I figured I'd get these things
fixed first.  This is based on top of Max's block branch.

Available as a tag at:
git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4

Since v3:
- check all qemu-iotests (patch 1)

001/4:[0012] [FC] 'qemu-io: Don't die on second open'
002/4:[----] [--] 'block: Guarantee that *file is set on bdrv_get_block_status()'
003/4:[----] [--] 'block: Simplify use of BDRV_BLOCK_RAW'
004/4:[----] [--] 'blkdebug: Support .bdrv_co_get_block_status'

Eric Blake (4):
  qemu-io: Don't die on second open
  block: Guarantee that *file is set on bdrv_get_block_status()
  block: Simplify use of BDRV_BLOCK_RAW
  blkdebug: Support .bdrv_co_get_block_status

 include/block/block.h      |  6 +++---
 block/blkdebug.c           | 11 +++++++++++
 block/commit.c             |  2 +-
 block/io.c                 |  5 +++--
 block/mirror.c             |  2 +-
 block/raw-format.c         |  2 +-
 block/vpc.c                |  2 +-
 qemu-io.c                  |  7 ++++---
 tests/qemu-iotests/060.out |  1 +
 tests/qemu-iotests/114.out |  5 +++--
 tests/qemu-iotests/153.out |  6 ++++++
 tests/qemu-iotests/177     |  3 +++
 tests/qemu-iotests/177.out |  5 +++++
 13 files changed, 43 insertions(+), 14 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v4 1/4] qemu-io: Don't die on second open

[Eric Blake]

Most callback commands in qemu-io return 0 to keep the interpreter
loop running, or 1 to quit immediately.  However, open_f() just
passed through the return value of openfile(), which has different
semantics of returning 0 if a file was opened, or 1 on any failure.

As a result of mixing the return semantics, we are forcing the
qemu-io interpreter to exit early on any failures, which is rather
annoying when some of the failures are obviously trying to give
the user a hint of how to proceed (if we didn't then kill qemu-io
out from under the user's feet):

$ qemu-io
qemu-io> open foo
qemu-io> open foo
file open already, try 'help close'
$ echo $?
0

In general, we WANT openfile() to report failures, since it is the
function used in the form 'qemu-io -c "$something" no_such_file'
for performing one or more -c options on a single file, and it is
not worth attempting $something if the file itself cannot be opened.
So the solution is to fix open_f() to always return 0 (when we are
in interactive mode, even failure to open should not end the
session), and save the return value of openfile() for command line
use in main().

Note, however, that we do have some qemu-iotests that do 'qemu-io
-c "open file" -c "$something"'; such tests will now proceed to
attempt $something whether or not the open succeeded, the same way
as if the two commands had been attempted in interactive mode.  As
such, the expected output for those tests has to be modified.  But it
also means that it is now possible to use -c close and have a single
qemu-io command line operate on more than one file even without
using interactive mode.  Although the '-c open' action is a subtle
change in behavior, remember that qemu-io is for debugging purposes,
so as long as it serves the needs of qemu-iotests while still being
reasonable for interactive use, it should not be a problem that we
are changing tests to the new behavior.

This has been awkward since at least as far back as commit
e3aff4f, in 2009.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>

---
v4: cover ALL qemu-iotests
v3: tweak commit message to distinguish that '-c open' has a subtle
new behavior from the command line
v2: fix open_f(), not openfile()
---
 qemu-io.c                  | 7 ++++---
 tests/qemu-iotests/060.out | 1 +
 tests/qemu-iotests/114.out | 5 +++--
 tests/qemu-iotests/153.out | 6 ++++++
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 8e38b28..8074656 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -230,13 +230,14 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
     qemu_opts_reset(&empty_opts);

     if (optind == argc - 1) {
-        return openfile(argv[optind], flags, writethrough, force_share, opts);
+        openfile(argv[optind], flags, writethrough, force_share, opts);
     } else if (optind == argc) {
-        return openfile(NULL, flags, writethrough, force_share, opts);
+        openfile(NULL, flags, writethrough, force_share, opts);
     } else {
         QDECREF(opts);
-        return qemuio_command_usage(&open_cmd);
+        qemuio_command_usage(&open_cmd);
     }
+    return 0;
 }

 static int quit_f(BlockBackend *blk, int argc, char **argv)
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 3bc1461..5ca3af4 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -21,6 +21,7 @@ Format specific information:
     refcount bits: 16
     corrupt: true
 can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write
+no file open, try 'help open'
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out
index b6d10e4..1a47a52 100644
--- a/tests/qemu-iotests/114.out
+++ b/tests/qemu-iotests/114.out
@@ -1,6 +1,6 @@
 QA output created by 114
-Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base 
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
 virtual size: 64M (67108864 bytes)
@@ -8,6 +8,7 @@ cluster_size: 65536
 backing file: TEST_DIR/t.IMGFMT.base
 backing file format: foo
 can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown driver 'foo'
+no file open, try 'help open'
 read 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index 5ba0b63..5b917b1 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -33,10 +33,12 @@ Is another process using the image?
 _qemu_io_wrapper -c open  TEST_DIR/t.qcow2 -c read 0 512
 can't open device TEST_DIR/t.qcow2: Failed to get "write" lock
 Is another process using the image?
+no file open, try 'help open'

 _qemu_io_wrapper -c open -r  TEST_DIR/t.qcow2 -c read 0 512
 can't open device TEST_DIR/t.qcow2: Failed to get shared "write" lock
 Is another process using the image?
+no file open, try 'help open'

 _qemu_img_wrapper info TEST_DIR/t.qcow2
 qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to get shared "write" lock
@@ -99,6 +101,7 @@ _qemu_io_wrapper -U -r -c read 0 512 TEST_DIR/t.qcow2

 _qemu_io_wrapper -c open -U TEST_DIR/t.qcow2 -c read 0 512
 can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+no file open, try 'help open'

 _qemu_io_wrapper -c open -r -U TEST_DIR/t.qcow2 -c read 0 512

@@ -166,6 +169,7 @@ _qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
 _qemu_io_wrapper -c open  TEST_DIR/t.qcow2 -c read 0 512
 can't open device TEST_DIR/t.qcow2: Failed to get "write" lock
 Is another process using the image?
+no file open, try 'help open'

 _qemu_io_wrapper -c open -r  TEST_DIR/t.qcow2 -c read 0 512

@@ -214,6 +218,7 @@ _qemu_io_wrapper -U -r -c read 0 512 TEST_DIR/t.qcow2

 _qemu_io_wrapper -c open -U TEST_DIR/t.qcow2 -c read 0 512
 can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+no file open, try 'help open'

 _qemu_io_wrapper -c open -r -U TEST_DIR/t.qcow2 -c read 0 512

@@ -313,6 +318,7 @@ _qemu_io_wrapper -U -r -c read 0 512 TEST_DIR/t.qcow2

 _qemu_io_wrapper -c open -U TEST_DIR/t.qcow2 -c read 0 512
 can't open device TEST_DIR/t.qcow2: force-share=on can only be used with read-only images
+no file open, try 'help open'

 _qemu_io_wrapper -c open -r -U TEST_DIR/t.qcow2 -c read 0 512

-- 
2.9.4

Re: [Qemu-devel] [PATCH v4 1/4] qemu-io: Don't die on second open

[John Snow]

On 06/05/2017 04:38 PM, Eric Blake wrote:
> Most callback commands in qemu-io return 0 to keep the interpreter
> loop running, or 1 to quit immediately.  However, open_f() just
> passed through the return value of openfile(), which has different
> semantics of returning 0 if a file was opened, or 1 on any failure.
> 
> As a result of mixing the return semantics, we are forcing the
> qemu-io interpreter to exit early on any failures, which is rather
> annoying when some of the failures are obviously trying to give
> the user a hint of how to proceed (if we didn't then kill qemu-io
> out from under the user's feet):
> 
> $ qemu-io
> qemu-io> open foo
> qemu-io> open foo
> file open already, try 'help close'
> $ echo $?
> 0
> 
> In general, we WANT openfile() to report failures, since it is the
> function used in the form 'qemu-io -c "$something" no_such_file'
> for performing one or more -c options on a single file, and it is
> not worth attempting $something if the file itself cannot be opened.
> So the solution is to fix open_f() to always return 0 (when we are
> in interactive mode, even failure to open should not end the
> session), and save the return value of openfile() for command line
> use in main().
> 
> Note, however, that we do have some qemu-iotests that do 'qemu-io
> -c "open file" -c "$something"'; such tests will now proceed to
> attempt $something whether or not the open succeeded, the same way
> as if the two commands had been attempted in interactive mode.  As
> such, the expected output for those tests has to be modified.  But it
> also means that it is now possible to use -c close and have a single
> qemu-io command line operate on more than one file even without
> using interactive mode.  Although the '-c open' action is a subtle
> change in behavior, remember that qemu-io is for debugging purposes,
> so as long as it serves the needs of qemu-iotests while still being
> reasonable for interactive use, it should not be a problem that we
> are changing tests to the new behavior.
> 
> This has been awkward since at least as far back as commit
> e3aff4f, in 2009.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>


Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH v4 2/4] block: Guarantee that *file is set on bdrv_get_block_status()

[Eric Blake]

We document that *file is valid if the return is not an error and
includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
when a driver (such as blkdebug) lacks a callback.  Messed up in
commit 67a0fd2 (v2.6), when we added the file parameter.

Enhance qemu-iotest 177 to cover this, using a sequence that would
print garbage or even SEGV, because it was dererefencing through
uninitialized memory.  [The resulting test output shows that we
have less-than-ideal block status from the blkdebug driver, but
that's a separate fix coming up soon.]

Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
enough to fix the crash, but we can go one step further: always
setting *file, even on error, means that a broken caller that
blindly dereferences file without checking for error is now more
likely to get a reliable SEGV instead of randomly acting on garbage,
making it easier to diagnose such buggy callers.  Adding an
assertion that file is set where expected doesn't hurt either.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>

---
v3: tweak commit message [Max], rebase test to pass
v2: drop redundant assignment
---
 block/io.c                 | 5 +++--
 tests/qemu-iotests/177     | 3 +++
 tests/qemu-iotests/177.out | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index ed31810..ce6cb0c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1736,6 +1736,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     int64_t n;
     int64_t ret, ret2;

+    *file = NULL;
     total_sectors = bdrv_nb_sectors(bs);
     if (total_sectors < 0) {
         return total_sectors;
@@ -1756,11 +1757,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
         if (bs->drv->protocol_name) {
             ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+            *file = bs;
         }
         return ret;
     }

-    *file = NULL;
     bdrv_inc_in_flight(bs);
     ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
                                             file);
@@ -1770,7 +1771,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     }

     if (ret & BDRV_BLOCK_RAW) {
-        assert(ret & BDRV_BLOCK_OFFSET_VALID);
+        assert(ret & BDRV_BLOCK_OFFSET_VALID && *file);
         ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
                                        *pnum, pnum, file);
         goto out;
diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177
index 2005c17..f8ed8fb 100755
--- a/tests/qemu-iotests/177
+++ b/tests/qemu-iotests/177
@@ -43,6 +43,7 @@ _supported_proto file
 CLUSTER_SIZE=1M
 size=128M
 options=driver=blkdebug,image.driver=qcow2
+nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG

 echo
 echo "== setting up files =="
@@ -106,6 +107,8 @@ function verify_io()
 }

 verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \
+    | _filter_qemu_img_map

 _check_test_img

diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index e887542..fcfbfa3 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352
 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 23068672/23068672 bytes at offset 111149056
 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          File
+0               0x8000000       json:{"image": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug", "align": "4k"}
 No errors were found on the image.
 *** done
-- 
2.9.4

Re: [Qemu-devel] [PATCH v4 2/4] block: Guarantee that *file is set on bdrv_get_block_status()

[John Snow]

On 06/05/2017 04:38 PM, Eric Blake wrote:
> We document that *file is valid if the return is not an error and
> includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract
> when a driver (such as blkdebug) lacks a callback.  Messed up in
> commit 67a0fd2 (v2.6), when we added the file parameter.
> 
> Enhance qemu-iotest 177 to cover this, using a sequence that would
> print garbage or even SEGV, because it was dererefencing through
> uninitialized memory.  [The resulting test output shows that we
> have less-than-ideal block status from the blkdebug driver, but
> that's a separate fix coming up soon.]
> 
> Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is
> enough to fix the crash, but we can go one step further: always
> setting *file, even on error, means that a broken caller that
> blindly dereferences file without checking for error is now more
> likely to get a reliable SEGV instead of randomly acting on garbage,
> making it easier to diagnose such buggy callers.  Adding an
> assertion that file is set where expected doesn't hurt either.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH v4 3/4] block: Simplify use of BDRV_BLOCK_RAW

[Eric Blake]

The lone caller that cares about a return of BDRV_BLOCK_RAW
(namely, io.c:bdrv_co_get_block_status) completely replaces the
return value, so there is no point in passing BDRV_BLOCK_DATA.

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

---
v3: further document BDRV_BLOCK_RAW
v2: fix subject, tweak commit message
---
 include/block/block.h | 6 +++---
 block/commit.c        | 2 +-
 block/mirror.c        | 2 +-
 block/raw-format.c    | 2 +-
 block/vpc.c           | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 9b355e9..0a60444 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -131,9 +131,9 @@ typedef struct HDGeometry {
  *                       layer (short for DATA || ZERO), set by block layer
  *
  * Internal flag:
- * BDRV_BLOCK_RAW: used internally to indicate that the request was
- *                 answered by a passthrough driver such as raw and that the
- *                 block layer should recompute the answer from bs->file.
+ * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
+ *                 that the block layer recompute the answer from the returned
+ *                 BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
  *
  * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
  * represent the offset in the returned BDS that is allocated for the
diff --git a/block/commit.c b/block/commit.c
index a3028b2..f56745b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -239,7 +239,7 @@ static int64_t coroutine_fn bdrv_commit_top_get_block_status(
 {
     *pnum = nb_sectors;
     *file = bs->backing->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
            (sector_num << BDRV_SECTOR_BITS);
 }

diff --git a/block/mirror.c b/block/mirror.c
index a2a9703..892d53a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1052,7 +1052,7 @@ static int64_t coroutine_fn bdrv_mirror_top_get_block_status(
 {
     *pnum = nb_sectors;
     *file = bs->backing->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
            (sector_num << BDRV_SECTOR_BITS);
 }

diff --git a/block/raw-format.c b/block/raw-format.c
index 36e6503..1136eba 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -259,7 +259,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
     *pnum = nb_sectors;
     *file = bs->file->bs;
     sector_num += s->offset / BDRV_SECTOR_SIZE;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
            (sector_num << BDRV_SECTOR_BITS);
 }

diff --git a/block/vpc.c b/block/vpc.c
index 4240ba9..b313c68 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -701,7 +701,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 |
+        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
                (sector_num << BDRV_SECTOR_BITS);
     }

-- 
2.9.4

Re: [Qemu-devel] [PATCH v4 3/4] block: Simplify use of BDRV_BLOCK_RAW

[Fam Zheng]

On Mon, 06/05 15:38, Eric Blake wrote:
> The lone caller that cares about a return of BDRV_BLOCK_RAW
> (namely, io.c:bdrv_co_get_block_status) completely replaces the
> return value, so there is no point in passing BDRV_BLOCK_DATA.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: further document BDRV_BLOCK_RAW
> v2: fix subject, tweak commit message
> ---
>  include/block/block.h | 6 +++---
>  block/commit.c        | 2 +-
>  block/mirror.c        | 2 +-
>  block/raw-format.c    | 2 +-
>  block/vpc.c           | 2 +-
>  5 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 9b355e9..0a60444 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -131,9 +131,9 @@ typedef struct HDGeometry {
>   *                       layer (short for DATA || ZERO), set by block layer
>   *
>   * Internal flag:
> - * BDRV_BLOCK_RAW: used internally to indicate that the request was
> - *                 answered by a passthrough driver such as raw and that the
> - *                 block layer should recompute the answer from bs->file.
> + * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request
> + *                 that the block layer recompute the answer from the returned
> + *                 BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.
>   *
>   * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
>   * represent the offset in the returned BDS that is allocated for the
> diff --git a/block/commit.c b/block/commit.c
> index a3028b2..f56745b 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -239,7 +239,7 @@ static int64_t coroutine_fn bdrv_commit_top_get_block_status(
>  {
>      *pnum = nb_sectors;
>      *file = bs->backing->bs;
> -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
>             (sector_num << BDRV_SECTOR_BITS);
>  }
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index a2a9703..892d53a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1052,7 +1052,7 @@ static int64_t coroutine_fn bdrv_mirror_top_get_block_status(
>  {
>      *pnum = nb_sectors;
>      *file = bs->backing->bs;
> -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
>             (sector_num << BDRV_SECTOR_BITS);
>  }
> 
> diff --git a/block/raw-format.c b/block/raw-format.c
> index 36e6503..1136eba 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -259,7 +259,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>      *pnum = nb_sectors;
>      *file = bs->file->bs;
>      sector_num += s->offset / BDRV_SECTOR_SIZE;
> -    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
>             (sector_num << BDRV_SECTOR_BITS);
>  }
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 4240ba9..b313c68 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -701,7 +701,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 |
> +        return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
>                 (sector_num << BDRV_SECTOR_BITS);
>      }
> 
> -- 
> 2.9.4
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH v4 4/4] blkdebug: Support .bdrv_co_get_block_status

[Eric Blake]

Without a passthrough status of BDRV_BLOCK_RAW, anything wrapped by
blkdebug appears 100% allocated as data.  Better is treating it the
same as the underlying file being wrapped.

Update iotest 177 for the new expected output.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>

---
v3: rebase to earlier changes
v2: tweak commit message
---
 block/blkdebug.c           | 11 +++++++++++
 tests/qemu-iotests/177.out |  5 ++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index a5196e8..1ad8d65 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -642,6 +642,16 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
     return bdrv_co_pdiscard(bs->file->bs, offset, count);
 }

+static int64_t coroutine_fn blkdebug_co_get_block_status(
+    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
+    BlockDriverState **file)
+{
+    *pnum = nb_sectors;
+    *file = bs->file->bs;
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+        (sector_num << BDRV_SECTOR_BITS);
+}
+
 static void blkdebug_close(BlockDriverState *bs)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -912,6 +922,7 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_co_flush_to_disk  = blkdebug_co_flush,
     .bdrv_co_pwrite_zeroes  = blkdebug_co_pwrite_zeroes,
     .bdrv_co_pdiscard       = blkdebug_co_pdiscard,
+    .bdrv_co_get_block_status = blkdebug_co_get_block_status,

     .bdrv_debug_event           = blkdebug_debug_event,
     .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out
index fcfbfa3..43a7778 100644
--- a/tests/qemu-iotests/177.out
+++ b/tests/qemu-iotests/177.out
@@ -46,6 +46,9 @@ read 30408704/30408704 bytes at offset 80740352
 read 23068672/23068672 bytes at offset 111149056
 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Offset          Length          File
-0               0x8000000       json:{"image": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug", "align": "4k"}
+0               0x800000        TEST_DIR/t.IMGFMT
+0x900000        0x2400000       TEST_DIR/t.IMGFMT
+0x3c00000       0x1100000       TEST_DIR/t.IMGFMT
+0x6a00000       0x1600000       TEST_DIR/t.IMGFMT
 No errors were found on the image.
 *** done
-- 
2.9.4

Re: [Qemu-devel] [PATCH v4 0/4] more blkdebug tweaks

[John Snow]

On 06/05/2017 04:38 PM, Eric Blake wrote:
> I found a crasher and some odd behavior while rebasing my
> bdrv_get_block_status series, so I figured I'd get these things
> fixed first.  This is based on top of Max's block branch.
> 
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4
> 
> Since v3:
> - check all qemu-iotests (patch 1)
> 
> 001/4:[0012] [FC] 'qemu-io: Don't die on second open'
> 002/4:[----] [--] 'block: Guarantee that *file is set on bdrv_get_block_status()'
> 003/4:[----] [--] 'block: Simplify use of BDRV_BLOCK_RAW'
> 004/4:[----] [--] 'blkdebug: Support .bdrv_co_get_block_status'
> 
> Eric Blake (4):
>   qemu-io: Don't die on second open
>   block: Guarantee that *file is set on bdrv_get_block_status()
>   block: Simplify use of BDRV_BLOCK_RAW
>   blkdebug: Support .bdrv_co_get_block_status
> 
>  include/block/block.h      |  6 +++---
>  block/blkdebug.c           | 11 +++++++++++
>  block/commit.c             |  2 +-
>  block/io.c                 |  5 +++--
>  block/mirror.c             |  2 +-
>  block/raw-format.c         |  2 +-
>  block/vpc.c                |  2 +-
>  qemu-io.c                  |  7 ++++---
>  tests/qemu-iotests/060.out |  1 +
>  tests/qemu-iotests/114.out |  5 +++--
>  tests/qemu-iotests/153.out |  6 ++++++
>  tests/qemu-iotests/177     |  3 +++
>  tests/qemu-iotests/177.out |  5 +++++
>  13 files changed, 43 insertions(+), 14 deletions(-)
> 

3,4:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v4 0/4] more blkdebug tweaks

[Kevin Wolf]

Am 05.06.2017 um 22:38 hat Eric Blake geschrieben:
> I found a crasher and some odd behavior while rebasing my
> bdrv_get_block_status series, so I figured I'd get these things
> fixed first.  This is based on top of Max's block branch.
> 
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4
> 
> Since v3:
> - check all qemu-iotests (patch 1)

Whoops, somehow I ended up applying and reviewing v4 while looking at
the v3 thread... Looks like there really aren't any missing test case
updates any more.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v4 0/4] more blkdebug tweaks

[Eric Blake]

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

On 06/06/2017 07:26 AM, Kevin Wolf wrote:
> Am 05.06.2017 um 22:38 hat Eric Blake geschrieben:
>> I found a crasher and some odd behavior while rebasing my
>> bdrv_get_block_status series, so I figured I'd get these things
>> fixed first.  This is based on top of Max's block branch.
>>
>> Available as a tag at:
>> git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4
>>
>> Since v3:
>> - check all qemu-iotests (patch 1)
> 
> Whoops, somehow I ended up applying and reviewing v4 while looking at
> the v3 thread... Looks like there really aren't any missing test case
> updates any more.
> 
> Thanks, applied to the block branch.

Did this get lost somehow?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH v4 0/4] more blkdebug tweaks

[Kevin Wolf]

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

Am 27.06.2017 um 18:01 hat Eric Blake geschrieben:
> On 06/06/2017 07:26 AM, Kevin Wolf wrote:
> > Am 05.06.2017 um 22:38 hat Eric Blake geschrieben:
> >> I found a crasher and some odd behavior while rebasing my
> >> bdrv_get_block_status series, so I figured I'd get these things
> >> fixed first.  This is based on top of Max's block branch.
> >>
> >> Available as a tag at:
> >> git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4
> >>
> >> Since v3:
> >> - check all qemu-iotests (patch 1)
> > 
> > Whoops, somehow I ended up applying and reviewing v4 while looking at
> > the v3 thread... Looks like there really aren't any missing test case
> > updates any more.
> > 
> > Thanks, applied to the block branch.
> 
> Did this get lost somehow?

No idea what happened there, but it looks like it. It's queued now.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]