[Qemu-devel] [PATCH v2 0/5] more blkdebug tweaks

[Qemu-devel] [PATCH v2 0/5] 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.

Since v1:
- patch 1: patch open_f instead of openfile [Fam]
- patch 2: drop redundant assignment
- patch 3: new
- patch 4, 5: tweak commit message

(no diff from v1, since I forgot to tag that one)

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

Eric Blake (5):
  qemu-io: Don't die on second open
  block: Guarantee that *file is set on bdrv_get_block_status()
  block: Allow NULL file for 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                 | 20 ++++++++++++--------
 block/mirror.c             |  5 ++---
 block/qcow2.c              |  4 +---
 block/raw-format.c         |  2 +-
 block/vpc.c                |  2 +-
 qemu-img.c                 | 10 ++++------
 qemu-io.c                  |  7 ++++---
 tests/qemu-iotests/177     |  3 +++
 tests/qemu-iotests/177.out |  5 +++++
 11 files changed, 45 insertions(+), 26 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v2 1/5] 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

Meanwhile, we WANT openfile() to report failures, as it is the
way that 'qemu-io -c "$something" no_such_file' knows to exit
early rather than attempting $something.  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().

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

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

---
v2: fix open_f(), not openfile()
---
 qemu-io.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 34fa8a1..b3febc2 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)
-- 
2.9.4

[Qemu-devel] [PATCH v2 2/5] 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 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>

---
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 fdd7485..8e6c3fe 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,11 +1770,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);
@@ -1783,7 +1784,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 v2 3/5] block: Allow NULL file for bdrv_get_block_status()

[Eric Blake]

Not all callers care about which BDS owns the mapping for a given
range of the file.  This patch merely simplifies the callers by
consolidating the logic in the common call point, while guaranteeing
a non-NULL file to all the driver callbacks, for no semantic change.

However, this will also set the stage for a future cleanup: when a
caller does not care about which BDS owns an offset, it would be
nice to allow the driver to optimize things to not have to return
BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
allocation (for example, it's fairly easy to create a qcow2 image
where consecutive guest addresses are not at consecutive host
addresses), the current contract requires bdrv_get_block_status()
to clamp *pnum to the limit where host addresses are no longer
consecutive, but allowing a NULL file means that *pnum could be
set to the full length of known-allocated data.

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

---
v2: new patch
---
 block/io.c     | 15 +++++++++------
 block/mirror.c |  3 +--
 block/qcow2.c  |  4 +---
 qemu-img.c     | 10 ++++------
 4 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/block/io.c b/block/io.c
index 8e6c3fe..eea74cb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
 {
     int64_t target_sectors, ret, nb_sectors, sector_num = 0;
     BlockDriverState *bs = child->bs;
-    BlockDriverState *file;
     int n;

     target_sectors = bdrv_nb_sectors(bs);
@@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
         if (nb_sectors <= 0) {
             return 0;
         }
-        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
+        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL);
         if (ret < 0) {
             error_report("error getting block status at sector %" PRId64 ": %s",
                          sector_num, strerror(-ret));
@@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData {
  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
  * beyond the end of the disk image it will be clamped.
  *
- * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
- * points to the BDS which the sector range is allocated in.
+ * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
+ * 'file' is non-NULL, then '*file' points to the BDS which the sector range
+ * is allocated in.
  */
 static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
                                                      int64_t sector_num,
@@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     int64_t total_sectors;
     int64_t n;
     int64_t ret, ret2;
+    BlockDriverState *tmpfile;

+    if (!file) {
+        file = &tmpfile;
+    }
     *file = NULL;
     total_sectors = bdrv_nb_sectors(bs);
     if (total_sectors < 0) {
@@ -1916,9 +1920,8 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,
 int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
                                    int nb_sectors, int *pnum)
 {
-    BlockDriverState *file;
     int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum,
-                                        &file);
+                                        NULL);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/mirror.c b/block/mirror.c
index e86f8f8..4563ba7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -392,7 +392,6 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     while (nb_chunks > 0 && sector_num < end) {
         int64_t ret;
         int io_sectors, io_sectors_acct;
-        BlockDriverState *file;
         enum MirrorMethod {
             MIRROR_METHOD_COPY,
             MIRROR_METHOD_ZERO,
@@ -402,7 +401,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         assert(!(sector_num % sectors_per_chunk));
         ret = bdrv_get_block_status_above(source, NULL, sector_num,
                                           nb_chunks * sectors_per_chunk,
-                                          &io_sectors, &file);
+                                          &io_sectors, NULL);
         if (ret < 0) {
             io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors);
         } else if (ret & BDRV_BLOCK_DATA) {
diff --git a/block/qcow2.c b/block/qcow2.c
index b3ba5da..8e9e29b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2448,7 +2448,6 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
                             uint32_t count)
 {
     int nr;
-    BlockDriverState *file;
     int64_t res;

     if (start + count > bs->total_sectors) {
@@ -2458,8 +2457,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
     if (!count) {
         return true;
     }
-    res = bdrv_get_block_status_above(bs, NULL, start, count,
-                                      &nr, &file);
+    res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL);
     return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
 }

diff --git a/qemu-img.c b/qemu-img.c
index 5aef8ef..cb78696 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1398,7 +1398,6 @@ static int img_compare(int argc, char **argv)

     for (;;) {
         int64_t status1, status2;
-        BlockDriverState *file;

         nb_sectors = sectors_to_process(total_sectors, sector_num);
         if (nb_sectors <= 0) {
@@ -1406,7 +1405,7 @@ static int img_compare(int argc, char **argv)
         }
         status1 = bdrv_get_block_status_above(bs1, NULL, sector_num,
                                               total_sectors1 - sector_num,
-                                              &pnum1, &file);
+                                              &pnum1, NULL);
         if (status1 < 0) {
             ret = 3;
             error_report("Sector allocation test failed for %s", filename1);
@@ -1416,7 +1415,7 @@ static int img_compare(int argc, char **argv)

         status2 = bdrv_get_block_status_above(bs2, NULL, sector_num,
                                               total_sectors2 - sector_num,
-                                              &pnum2, &file);
+                                              &pnum2, NULL);
         if (status2 < 0) {
             ret = 3;
             error_report("Sector allocation test failed for %s", filename2);
@@ -1615,15 +1614,14 @@ 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;
         if (s->target_has_backing) {
             ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
                                         sector_num - src_cur_offset,
-                                        n, &n, &file);
+                                        n, &n, NULL);
         } else {
             ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
                                               sector_num - src_cur_offset,
-                                              n, &n, &file);
+                                              n, &n, NULL);
         }
         if (ret < 0) {
             return ret;
-- 
2.9.4

[Qemu-devel] [PATCH v2 4/5] 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>

---
v2: fix subject, tweak commit message
---
 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 4563ba7..432d58d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1046,7 +1046,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 v2 5/5] 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>

---
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 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 v2 1/5] qemu-io: Don't die on second open

[Fam Zheng]

On Wed, 05/24 15:28, 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
> 
> Meanwhile, we WANT openfile() to report failures, as it is the
> way that 'qemu-io -c "$something" no_such_file' knows to exit
> early rather than attempting $something.  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().
> 
> This has been awkward since at least as far back as commit
> e3aff4f, in 2009.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: fix open_f(), not openfile()
> ---
>  qemu-io.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-io.c b/qemu-io.c
> index 34fa8a1..b3febc2 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)
> -- 
> 2.9.4
> 
> 

This is better, thanks!

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

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

[Fam Zheng]

On Wed, 05/24 15:28, 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.
> 
> 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>
> 
> ---
> 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 fdd7485..8e6c3fe 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,11 +1770,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);
> @@ -1783,7 +1784,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
> 

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

Re: [Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

[Fam Zheng]

On Wed, 05/24 15:28, Eric Blake wrote:
> Not all callers care about which BDS owns the mapping for a given
> range of the file.  This patch merely simplifies the callers by
> consolidating the logic in the common call point, while guaranteeing
> a non-NULL file to all the driver callbacks, for no semantic change.
> 
> However, this will also set the stage for a future cleanup: when a
> caller does not care about which BDS owns an offset, it would be
> nice to allow the driver to optimize things to not have to return
> BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
> allocation (for example, it's fairly easy to create a qcow2 image
> where consecutive guest addresses are not at consecutive host
> addresses), the current contract requires bdrv_get_block_status()
> to clamp *pnum to the limit where host addresses are no longer
> consecutive, but allowing a NULL file means that *pnum could be
> set to the full length of known-allocated data.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: new patch

Yes. any particular reason why this patch is useful, besides simplifying
callers?


> ---
>  block/io.c     | 15 +++++++++------
>  block/mirror.c |  3 +--
>  block/qcow2.c  |  4 +---
>  qemu-img.c     | 10 ++++------
>  4 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 8e6c3fe..eea74cb 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>  {
>      int64_t target_sectors, ret, nb_sectors, sector_num = 0;
>      BlockDriverState *bs = child->bs;
> -    BlockDriverState *file;
>      int n;
> 
>      target_sectors = bdrv_nb_sectors(bs);
> @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>          if (nb_sectors <= 0) {
>              return 0;
>          }
> -        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL);
>          if (ret < 0) {
>              error_report("error getting block status at sector %" PRId64 ": %s",
>                           sector_num, strerror(-ret));
> @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData {
>   * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
>   * beyond the end of the disk image it will be clamped.
>   *
> - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
> - * points to the BDS which the sector range is allocated in.
> + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
> + * 'file' is non-NULL, then '*file' points to the BDS which the sector range
> + * is allocated in.

Sounds good.

>   */
>  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int64_t sector_num,
> @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>      int64_t total_sectors;
>      int64_t n;
>      int64_t ret, ret2;
> +    BlockDriverState *tmpfile;
> 
> +    if (!file) {
> +        file = &tmpfile;
> +    }

I don't like this hunk. Instead, how about replacing all "*file = ..." with
"tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning?

>      *file = NULL;
>      total_sectors = bdrv_nb_sectors(bs);
>      if (total_sectors < 0) {
> @@ -1916,9 +1920,8 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,
>  int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                                     int nb_sectors, int *pnum)
>  {
> -    BlockDriverState *file;
>      int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum,
> -                                        &file);
> +                                        NULL);
>      if (ret < 0) {
>          return ret;
>      }
> diff --git a/block/mirror.c b/block/mirror.c
> index e86f8f8..4563ba7 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -392,7 +392,6 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>      while (nb_chunks > 0 && sector_num < end) {
>          int64_t ret;
>          int io_sectors, io_sectors_acct;
> -        BlockDriverState *file;
>          enum MirrorMethod {
>              MIRROR_METHOD_COPY,
>              MIRROR_METHOD_ZERO,
> @@ -402,7 +401,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          assert(!(sector_num % sectors_per_chunk));
>          ret = bdrv_get_block_status_above(source, NULL, sector_num,
>                                            nb_chunks * sectors_per_chunk,
> -                                          &io_sectors, &file);
> +                                          &io_sectors, NULL);
>          if (ret < 0) {
>              io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors);
>          } else if (ret & BDRV_BLOCK_DATA) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b3ba5da..8e9e29b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2448,7 +2448,6 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>                              uint32_t count)
>  {
>      int nr;
> -    BlockDriverState *file;
>      int64_t res;
> 
>      if (start + count > bs->total_sectors) {
> @@ -2458,8 +2457,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start,
>      if (!count) {
>          return true;
>      }
> -    res = bdrv_get_block_status_above(bs, NULL, start, count,
> -                                      &nr, &file);
> +    res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL);
>      return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count;
>  }
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 5aef8ef..cb78696 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1398,7 +1398,6 @@ static int img_compare(int argc, char **argv)
> 
>      for (;;) {
>          int64_t status1, status2;
> -        BlockDriverState *file;
> 
>          nb_sectors = sectors_to_process(total_sectors, sector_num);
>          if (nb_sectors <= 0) {
> @@ -1406,7 +1405,7 @@ static int img_compare(int argc, char **argv)
>          }
>          status1 = bdrv_get_block_status_above(bs1, NULL, sector_num,
>                                                total_sectors1 - sector_num,
> -                                              &pnum1, &file);
> +                                              &pnum1, NULL);
>          if (status1 < 0) {
>              ret = 3;
>              error_report("Sector allocation test failed for %s", filename1);
> @@ -1416,7 +1415,7 @@ static int img_compare(int argc, char **argv)
> 
>          status2 = bdrv_get_block_status_above(bs2, NULL, sector_num,
>                                                total_sectors2 - sector_num,
> -                                              &pnum2, &file);
> +                                              &pnum2, NULL);
>          if (status2 < 0) {
>              ret = 3;
>              error_report("Sector allocation test failed for %s", filename2);
> @@ -1615,15 +1614,14 @@ 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;
>          if (s->target_has_backing) {
>              ret = bdrv_get_block_status(blk_bs(s->src[src_cur]),
>                                          sector_num - src_cur_offset,
> -                                        n, &n, &file);
> +                                        n, &n, NULL);
>          } else {
>              ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL,
>                                                sector_num - src_cur_offset,
> -                                              n, &n, &file);
> +                                              n, &n, NULL);
>          }
>          if (ret < 0) {
>              return ret;
> -- 
> 2.9.4
> 

Otherwise looks good.

Fam

Re: [Qemu-devel] [PATCH v2 4/5] block: Simplify use of BDRV_BLOCK_RAW

[Fam Zheng]

On Wed, 05/24 15:28, 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>
> 
> ---
> v2: fix subject, tweak commit message
> ---
>  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 4563ba7..432d58d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1046,7 +1046,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
> 
> 

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

Re: [Qemu-devel] [PATCH v2 5/5] blkdebug: Support .bdrv_co_get_block_status

[Fam Zheng]

On Wed, 05/24 15:28, Eric Blake wrote:
> 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>
> 
> ---
> 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 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
> 
> 

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

Re: [Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

[Eric Blake]

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

On 05/25/2017 01:34 AM, Fam Zheng wrote:
> On Wed, 05/24 15:28, Eric Blake wrote:
>> Not all callers care about which BDS owns the mapping for a given
>> range of the file.  This patch merely simplifies the callers by
>> consolidating the logic in the common call point, while guaranteeing
>> a non-NULL file to all the driver callbacks, for no semantic change.
>>
>> However, this will also set the stage for a future cleanup: when a
>> caller does not care about which BDS owns an offset, it would be
>> nice to allow the driver to optimize things to not have to return
>> BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
>> allocation (for example, it's fairly easy to create a qcow2 image
>> where consecutive guest addresses are not at consecutive host
>> addresses), the current contract requires bdrv_get_block_status()
>> to clamp *pnum to the limit where host addresses are no longer
>> consecutive, but allowing a NULL file means that *pnum could be
>> set to the full length of known-allocated data.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v2: new patch
> 
> Yes. any particular reason why this patch is useful, besides simplifying
> callers?

I guess it is not directly useful to this series. I can delay it to
later, and include it as part of my v2 of changing
bdrv_get_block_status() to be byte-based.  As mentioned in the commit
message, it opens the door for a potential optimization for callers that
don't care about offsets, but only allocation status, so that they can
traverse the entire device address space with fewer queries.

I'm fine if we want to just go with 1,2,4,5 for now.

>> @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>      int64_t total_sectors;
>>      int64_t n;
>>      int64_t ret, ret2;
>> +    BlockDriverState *tmpfile;
>>
>> +    if (!file) {
>> +        file = &tmpfile;
>> +    }
> 
> I don't like this hunk. Instead, how about replacing all "*file = ..." with
> "tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning?

Can do, particularly if I delay this patch to a later series, and we go
with the rest now.

-- 
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 v2 0/5] more blkdebug tweaks

[Fam Zheng]

On Wed, 05/24 15:28, 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.

For patches 1,2,4,5: Acked-by: Fam Zheng <famz@redhat.com>

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

[Max Reitz]

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

On 2017-05-24 22:28, 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
> 
> Meanwhile, we WANT openfile() to report failures, as it is the
> way that 'qemu-io -c "$something" no_such_file' knows to exit
> early rather than attempting $something.  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().
> 
> This has been awkward since at least as far back as commit
> e3aff4f, in 2009.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

This still makes qemu-io -c "$something" no_such_file fail, right. But
now qemu-io -c "open no_such_file" -c "$something" will execute
$something; and I'm not sure we can convert all -c open users to not use
that command (maybe we can, now that we have --image-opts).

I don't think it's absolutely necessary for -c open to exit on error,
but you seem to do, if I understand your penultimate paragraph
correctly. :-)

Max


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

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

[Max Reitz]

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

On 2017-05-24 22:28, 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.

Not sure if I'd call that "broken", as in the part participle of "break"
because there was never any breaking. It just didn't abide by the
contract from the start.

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

Anyway,

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


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

Re: [Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

[Max Reitz]

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

On 2017-05-25 08:34, Fam Zheng wrote:
> On Wed, 05/24 15:28, Eric Blake wrote:
>> Not all callers care about which BDS owns the mapping for a given
>> range of the file.  This patch merely simplifies the callers by
>> consolidating the logic in the common call point, while guaranteeing
>> a non-NULL file to all the driver callbacks, for no semantic change.
>>
>> However, this will also set the stage for a future cleanup: when a
>> caller does not care about which BDS owns an offset, it would be
>> nice to allow the driver to optimize things to not have to return
>> BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
>> allocation (for example, it's fairly easy to create a qcow2 image
>> where consecutive guest addresses are not at consecutive host
>> addresses), the current contract requires bdrv_get_block_status()
>> to clamp *pnum to the limit where host addresses are no longer
>> consecutive, but allowing a NULL file means that *pnum could be
>> set to the full length of known-allocated data.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v2: new patch
> 
> Yes. any particular reason why this patch is useful, besides simplifying
> callers?
> 
> 
>> ---
>>  block/io.c     | 15 +++++++++------
>>  block/mirror.c |  3 +--
>>  block/qcow2.c  |  4 +---
>>  qemu-img.c     | 10 ++++------
>>  4 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 8e6c3fe..eea74cb 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>>  {
>>      int64_t target_sectors, ret, nb_sectors, sector_num = 0;
>>      BlockDriverState *bs = child->bs;
>> -    BlockDriverState *file;
>>      int n;
>>
>>      target_sectors = bdrv_nb_sectors(bs);
>> @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
>>          if (nb_sectors <= 0) {
>>              return 0;
>>          }
>> -        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
>> +        ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL);
>>          if (ret < 0) {
>>              error_report("error getting block status at sector %" PRId64 ": %s",
>>                           sector_num, strerror(-ret));
>> @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData {
>>   * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
>>   * beyond the end of the disk image it will be clamped.
>>   *
>> - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
>> - * points to the BDS which the sector range is allocated in.
>> + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
>> + * 'file' is non-NULL, then '*file' points to the BDS which the sector range
>> + * is allocated in.
> 
> Sounds good.
> 
>>   */
>>  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>                                                       int64_t sector_num,
>> @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>>      int64_t total_sectors;
>>      int64_t n;
>>      int64_t ret, ret2;
>> +    BlockDriverState *tmpfile;
>>
>> +    if (!file) {
>> +        file = &tmpfile;
>> +    }
> 
> I don't like this hunk. Instead, how about replacing all "*file = ..." with
> "tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning?

Sounds fine to me, but in that case I'd like to request a different
variable name. Maybe do what we have with errp/local_error and make it
local_file or something?

Max


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

Re: [Qemu-devel] [PATCH v2 4/5] block: Simplify use of BDRV_BLOCK_RAW

[Max Reitz]

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

On 2017-05-24 22:28, 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>

Technically true, but conflicts with the documentation in block.h.

If you really want this changed, you should document in block.h what
exactly it is that BDRV_BLOCK_RAW does.

Max


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

Re: [Qemu-devel] [PATCH v2 5/5] blkdebug: Support .bdrv_co_get_block_status

[Max Reitz]

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

On 2017-05-24 22:28, Eric Blake wrote:
> 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>
> 
> ---
> v2: tweak commit message
> ---
>  block/blkdebug.c           | 11 +++++++++++
>  tests/qemu-iotests/177.out |  5 ++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)

Well, depends on the previous patch, but:

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


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

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

[Eric Blake]

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

On 05/31/2017 09:18 AM, Max Reitz wrote:
> On 2017-05-24 22:28, 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
>>
>> Meanwhile, we WANT openfile() to report failures, as it is the
>> way that 'qemu-io -c "$something" no_such_file' knows to exit
>> early rather than attempting $something.  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().
>>
>> This has been awkward since at least as far back as commit
>> e3aff4f, in 2009.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> This still makes qemu-io -c "$something" no_such_file fail, right. But
> now qemu-io -c "open no_such_file" -c "$something" will execute
> $something; and I'm not sure we can convert all -c open users to not use
> that command (maybe we can, now that we have --image-opts).

Oh, so we do have some uses of -c "open..." -c "$something" (iotest 103
for example).

What happens during "$something" if there was no file currently open? It
may be a command-by-command behavior.  But as long as we get graceful
secondary failures rather than crashes for those subsequent $somethings,
we may be okay.

> 
> I don't think it's absolutely necessary for -c open to exit on error,
> but you seem to do, if I understand your penultimate paragraph
> correctly. :-)

I don't think it's absolutely necessary either.  You're right that this
patch is just worried about 'qemu-img name', not 'qemu-img -c "open
name"'.  So maybe all I need to do is update the commit message to
document that the change in semantics to -c "open..." is a side-effect,
that may or may not be changed in a followup patch?

-- 
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 v2 1/5] qemu-io: Don't die on second open

[Max Reitz]

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

On 2017-05-31 17:12, Eric Blake wrote:
> On 05/31/2017 09:18 AM, Max Reitz wrote:
>> On 2017-05-24 22:28, 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
>>>
>>> Meanwhile, we WANT openfile() to report failures, as it is the
>>> way that 'qemu-io -c "$something" no_such_file' knows to exit
>>> early rather than attempting $something.  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().
>>>
>>> This has been awkward since at least as far back as commit
>>> e3aff4f, in 2009.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> This still makes qemu-io -c "$something" no_such_file fail, right. But
>> now qemu-io -c "open no_such_file" -c "$something" will execute
>> $something; and I'm not sure we can convert all -c open users to not use
>> that command (maybe we can, now that we have --image-opts).
> 
> Oh, so we do have some uses of -c "open..." -c "$something" (iotest 103
> for example).
> 
> What happens during "$something" if there was no file currently open? It
> may be a command-by-command behavior.  But as long as we get graceful
> secondary failures rather than crashes for those subsequent $somethings,
> we may be okay.

Sure, but this is arguing against "we want [qemu-io no_such_file] to
exit early". :-)

>> I don't think it's absolutely necessary for -c open to exit on error,
>> but you seem to do, if I understand your penultimate paragraph
>> correctly. :-)
> 
> I don't think it's absolutely necessary either.  You're right that this
> patch is just worried about 'qemu-img name', not 'qemu-img -c "open
> name"'.  So maybe all I need to do is update the commit message to
> document that the change in semantics to -c "open..." is a side-effect,
> that may or may not be changed in a followup patch?

If you're OK with that, I am, too.

Max


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