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

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

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

 block/blkdebug.c           | 11 +++++++++++
 block/commit.c             |  2 +-
 block/io.c                 |  4 +++-
 block/mirror.c             |  2 +-
 block/raw-format.c         |  2 +-
 block/vpc.c                |  2 +-
 qemu-io.c                  |  2 +-
 tests/qemu-iotests/177     |  3 +++
 tests/qemu-iotests/177.out |  5 +++++
 9 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.9.4

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

[Eric Blake]

Failure to open a file in qemu-io should normally return 1 on
failure to end the command loop, on the presumption that when
batching commands all on the command line, failure to open means
nothing further can be attempted. But when executing qemu-io
interactively, there is a special case: if open is executed a
second time, we print a hint that the user should try the
interactive 'close' first.  But the hint is useless if we don't
actually LET them try 'close'.

This has been awkward since at least as far back as commit
43642b3, in 2011 (probably earlier, but git blame has a harder
time going past the file renames at that point).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-io.c b/qemu-io.c
index 34fa8a1..0c82dac 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -63,7 +63,7 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share,
     if (qemuio_blk) {
         error_report("file open already, try 'help close'");
         QDECREF(opts);
-        return 1;
+        return 0;
     }

     if (force_share) {
-- 
2.9.4

[Qemu-devel] [PATCH 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.  Broken 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 only when setting 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 caller is no longer dereferencing
uninitialized memory, so that we are more likely to get a reliable
SEGV instead of randomly acting on garbage.  Adding an assertion
doesn't hurt either.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/io.c                 | 4 +++-
 tests/qemu-iotests/177     | 3 +++
 tests/qemu-iotests/177.out | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index fdd7485..164a82b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1749,6 +1749,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;
@@ -1769,6 +1770,7 @@ 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;
     }
@@ -1783,7 +1785,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..b754ed4 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       blkdebug::TEST_DIR/t.IMGFMT
 No errors were found on the image.
 *** done
-- 
2.9.4

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

[Eric Blake]

The lone caller that cares about a return of BDRV_BLOCK_RAW
completely replaces the return value, so there is no point in
passing BDRV_BLOCK_DATA.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/commit.c     | 2 +-
 block/mirror.c     | 2 +-
 block/raw-format.c | 2 +-
 block/vpc.c        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 76a0d98..cf662ba 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 e86f8f8..970c9f2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1047,7 +1047,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 ecfee77..048504b 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

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

For a quick manual test, compare this sequence, pre- and post-patch:
$ qemu-img create -f qcow2 file 10M
$ qemu-io -f qcow2 -c 'w 1m' file
$ qemu-img map -f qcow2 file
$ qemu-img map --image-opts driver=blkdebug,image.driver=qcow2,\
image.file.driver=file,image.file.filename=file

Update iotest 177 for the new expected output.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 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 b754ed4..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       blkdebug::TEST_DIR/t.IMGFMT
+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 3/4] block: Simplify use of BDRV_BLOCK_RAW.

[Eric Blake]

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

On 05/18/2017 09:32 PM, Eric Blake wrote:

Not sure why I included a trailing dot in the subject line; a maintainer
can remove that if desired.

> The lone caller that cares about a return of BDRV_BLOCK_RAW
> completely replaces the return value, so there is no point in
> passing BDRV_BLOCK_DATA.

For that matter, I could have mentioned that the lone caller is
io.c:bdrv_co_get_block_status().

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
-- 
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] [PATCH 1/4] qemu-io: Don't die on second open

[Fam Zheng]

On Thu, 05/18 21:32, Eric Blake wrote:
> Failure to open a file in qemu-io should normally return 1 on
> failure to end the command loop, on the presumption that when
> batching commands all on the command line, failure to open means
> nothing further can be attempted. But when executing qemu-io
> interactively, there is a special case: if open is executed a
> second time, we print a hint that the user should try the
> interactive 'close' first.  But the hint is useless if we don't
> actually LET them try 'close'.
> 
> This has been awkward since at least as far back as commit
> 43642b3, in 2011 (probably earlier, but git blame has a harder
> time going past the file renames at that point).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-io.c b/qemu-io.c
> index 34fa8a1..0c82dac 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share,
>      if (qemuio_blk) {
>          error_report("file open already, try 'help close'");
>          QDECREF(opts);
> -        return 1;
> +        return 0;
>      }
> 
>      if (force_share) {
> -- 
> 2.9.4
> 
> 

Hmm, failure is failure, why is "return 0" better than "return 1"?

If the control flow in the caller has a problem, fix it there? Specifically, I
don't think failed interactive open need to exit program, at all.

Fam

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

[Eric Blake]

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

On 05/24/2017 01:28 AM, Fam Zheng wrote:
> On Thu, 05/18 21:32, Eric Blake wrote:
>> Failure to open a file in qemu-io should normally return 1 on
>> failure to end the command loop, on the presumption that when
>> batching commands all on the command line, failure to open means
>> nothing further can be attempted. But when executing qemu-io
>> interactively, there is a special case: if open is executed a
>> second time, we print a hint that the user should try the
>> interactive 'close' first.  But the hint is useless if we don't
>> actually LET them try 'close'.
>>
>> This has been awkward since at least as far back as commit
>> 43642b3, in 2011 (probably earlier, but git blame has a harder
>> time going past the file renames at that point).
>>

>> @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share,
>>      if (qemuio_blk) {
>>          error_report("file open already, try 'help close'");
>>          QDECREF(opts);
>> -        return 1;
>> +        return 0;
>>      }
>>
>>      if (force_share) {
>> -- 
>> 2.9.4
>>
>>
> 
> Hmm, failure is failure, why is "return 0" better than "return 1"?

"return 0" is what tells the caller to keep interpreting commands.
"return 1" is what tells the caller to exit immediately.  Most qemu-io
commands return 0 always.  Only a few need to return 1 ("quit"
absolutely needs to, and we are arguing about whether "open" should do
so).  Pre-patch, "open" always failed with 1, exiting the interpreter
immediately with nonzero status.  Which is wrong if we print a help
message on the second attempt at open (since a second attempt is only
possible in interactive mode), but DOES match what you want to have
happen on a first open attempt from the command line:

$ qemu-io unreadable_file

But you are arguing that in batch mode, when you do:

$ qemu-io
qemu-io> open unreadable_file

that you want to keep things in qemu-io, rather than exiting
immediately.  If so, then we need to make openfile() return 0 always,
and instead teach the caller to check whether it is invoked in command
line mode (cause non-zero exit status if nothing was opened) or in
interactive mode (keep qemu-io running, to let the user try to open
something else).  In command line mode, the caller should then use the
status of whether a file was actually opened (rather than the return
code) when dealing with the call to openfile() for the command line
argument.

That's a bigger change; but I can try it if you think it is worth it.

> 
> If the control flow in the caller has a problem, fix it there? Specifically, I
> don't think failed interactive open need to exit program, at all.
> 
> Fam
> 

-- 
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] [PATCH 2/4] block: Guarantee that *file is set on bdrv_get_block_status()

[Eric Blake]

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

On 05/18/2017 09:32 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.  Broken in
> commit 67a0fd2 (v2.6), when we added the file parameter.
> 

> +++ b/block/io.c
> @@ -1749,6 +1749,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;
> @@ -1769,6 +1770,7 @@ 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;
>      }

Continuing context:

    *file = NULL;
    ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors,
pnum,
                                            file);


Guess I need a v2, to remove the now-redundant second initialization of
*file.

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