[Qemu-devel] [PATCH for-2.9 0/3] Fix bdrv_is_allocated usage bugs

[Qemu-devel] [PATCH for-2.9 0/3] Fix bdrv_is_allocated usage bugs

[Eric Blake]

bdrv_is_allocated() returns tri-state, not just bool, although
there were several callers using it as a bool.  Fix them to
either propagate the error or to document why treatment of
failure like allocation is okay.

[Found during a larger effort to convert bdrv_get_block_status
to be byte-based for 2.10]

Eric Blake (3):
  backup: React to bdrv_is_allocated() errors
  vvfat: React to bdrv_is_allocated() errors
  migration: Document handling of bdrv_is_allocated() errors

 block/backup.c    | 14 ++++++++++----
 block/vvfat.c     | 22 +++++++++++++++++++---
 migration/block.c |  2 ++
 3 files changed, 31 insertions(+), 7 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] backup: React to bdrv_is_allocated() errors

[Eric Blake]

If bdrv_is_allocated() fails, we should immediately do the backup
error action, rather than attempting backup_do_cow() (although
that will likely fail too).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/backup.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index d3d20db..a4fb288 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -468,13 +468,14 @@ static void coroutine_fn backup_run(void *opaque)
         /* Both FULL and TOP SYNC_MODE's require copying.. */
         for (; start < end; start++) {
             bool error_is_read;
+            int alloced = 0;
+
             if (yield_and_check(job)) {
                 break;
             }

             if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
                 int i, n;
-                int alloced = 0;

                 /* Check to see if these blocks are already in the
                  * backing file. */
@@ -492,7 +493,7 @@ static void coroutine_fn backup_run(void *opaque)
                                 sectors_per_cluster - i, &n);
                     i += n;

-                    if (alloced == 1 || n == 0) {
+                    if (alloced || n == 0) {
                         break;
                     }
                 }
@@ -504,8 +505,13 @@ static void coroutine_fn backup_run(void *opaque)
                 }
             }
             /* FULL sync mode we copy the whole drive. */
-            ret = backup_do_cow(job, start * sectors_per_cluster,
-                                sectors_per_cluster, &error_is_read, false);
+            if (alloced < 0) {
+                ret = alloced;
+            } else {
+                ret = backup_do_cow(job, start * sectors_per_cluster,
+                                    sectors_per_cluster, &error_is_read,
+                                    false);
+            }
             if (ret < 0) {
                 /* Depending on error action, fail now or retry cluster */
                 BlockErrorAction action =
-- 
2.9.3

[Qemu-devel] [PATCH 2/3] vvfat: React to bdrv_is_allocated() errors

[Eric Blake]

If bdrv_is_allocated() fails, we should react to that failure.
For 2 of the 3 callers, reporting the error was easy.  But in
cluster_was_modified() and its lone caller
get_cluster_count_for_direntry(), it's rather invasive to update
the logic to pass the error back; so there, I went with merely
documenting the issue by changing the return type to bool (in
all likelihood, treating the cluster as modified will then
trigger a read which will also fail, and eventually get to an
error - but given the appalling number of abort() calls in this
code, I'm not making it any worse).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/vvfat.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index aa61c32..af5153d 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1394,7 +1394,13 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
 	   return -1;
 	if (s->qcow) {
 	    int n;
-            if (bdrv_is_allocated(s->qcow->bs, sector_num, nb_sectors-i, &n)) {
+            int ret;
+            ret = bdrv_is_allocated(s->qcow->bs, sector_num,
+                                    nb_sectors - i, &n);
+            if (ret < 0) {
+                return ret;
+            }
+            if (ret) {
                 DLOG(fprintf(stderr, "sectors %d+%d allocated\n",
                              (int)sector_num, n));
                 if (bdrv_read(s->qcow, sector_num, buf + i * 0x200, n)) {
@@ -1668,7 +1674,8 @@ static inline uint32_t modified_fat_get(BDRVVVFATState* s,
     }
 }

-static inline int cluster_was_modified(BDRVVVFATState* s, uint32_t cluster_num)
+static inline bool cluster_was_modified(BDRVVVFATState *s,
+                                        uint32_t cluster_num)
 {
     int was_modified = 0;
     int i, dummy;
@@ -1683,7 +1690,13 @@ static inline int cluster_was_modified(BDRVVVFATState* s, uint32_t cluster_num)
                                          1, &dummy);
     }

-    return was_modified;
+    /*
+     * Note that this treats failures to learn allocation status the
+     * same as if an allocation has occurred.  It's as safe as
+     * anything else, given that a failure to learn allocation status
+     * will probably result in more failures.
+     */
+    return !!was_modified;
 }

 static const char* get_basename(const char* path)
@@ -1833,6 +1846,9 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
                     int res;

                     res = bdrv_is_allocated(s->qcow->bs, offset + i, 1, &dummy);
+                    if (res < 0) {
+                        return -1;
+                    }
                     if (!res) {
                         res = vvfat_read(s->bs, offset, s->cluster_buffer, 1);
                         if (res) {
-- 
2.9.3

[Qemu-devel] [PATCH 3/3] migration: Document handling of bdrv_is_allocated() errors

[Eric Blake]

Migration is the only code left in the tree that does not react
to bdrv_is_allocated() failures.  But as there is no useful way
to react to the failure, and we are merely skipping unallocated
sectors on success, just document that our choice of handling
is intended.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 migration/block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/block.c b/migration/block.c
index 1941bc2..6741228 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -276,6 +276,8 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
     if (bmds->shared_base) {
         qemu_mutex_lock_iothread();
         aio_context_acquire(blk_get_aio_context(bb));
+        /* Skip unallocated sectors; intentionally treats failure as
+         * an allocated sector */
         while (cur_sector < total_sectors &&
                !bdrv_is_allocated(blk_bs(bb), cur_sector,
                                   MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.9 0/3] Fix bdrv_is_allocated usage bugs

[Kevin Wolf]

Am 08.03.2017 um 22:34 hat Eric Blake geschrieben:
> bdrv_is_allocated() returns tri-state, not just bool, although
> there were several callers using it as a bool.  Fix them to
> either propagate the error or to document why treatment of
> failure like allocation is okay.
> 
> [Found during a larger effort to convert bdrv_get_block_status
> to be byte-based for 2.10]

Thanks, applied to the block branch.

Kevin