Re: [Qemu-devel] [PATCH] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap

[Kevin Wolf]

Am 03.04.2014 um 16:44 hat Max Reitz geschrieben:
> On 02.04.2014 08:04, Fam Zheng wrote:
> >bdrv_getlength could fail, check the return value before using it.
> >
> >Signed-off-by: Fam Zheng <address@hidden>
> >---
> >  block-migration.c     | 28 ++++++++++++++++++++++++----
> >  block.c               | 10 ++++++++--
> >  block/mirror.c        |  5 ++++-
> >  include/block/block.h |  3 ++-
> >  4 files changed, 38 insertions(+), 8 deletions(-)
> >
> >diff --git a/block-migration.c b/block-migration.c
> >index 897fdba..62cd597 100644
> >--- a/block-migration.c
> >+++ b/block-migration.c
> >@@ -310,13 +310,26 @@ static int mig_save_device_bulk(QEMUFile *f, 
> >BlkMigDevState *bmds)
> >  /* Called with iothread lock taken.  */
> >-static void set_dirty_tracking(void)
> >+static int set_dirty_tracking(void)
> >  {
> >      BlkMigDevState *bmds;
> >      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> >-        bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE);
> >+        bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
> >+                                                      NULL);
> >+        if (!bmds->dirty_bitmap) {
> >+            goto fail;
> >+        }
> >+    }
> >+    return 0;
> >+
> >+fail:
> >+    QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> >+        if (bmds->dirty_bitmap) {
> >+            bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
> >+        }
> >      }
> >+    return -1;
> 
> Through block_save_setup(), this ends up as f->last_error which is
> at least in qemu_loadvm_state() interpreted as -errno (or rather,
> that function generally returns -errno and in this case, it returns
> f->last_error). I know that it is not easy to find a correct error
> code here, but EPERM seems rather unfitting; even EIO would be
> better, in my opinion.
> 
> Anyway, this is not really bad, so:
> 
> Reviewed-by: Max Reitz <address@hidden>

Let's not add new cases of -1 interpreted as negative errno. Patches
that improve error handling should do it right.

> >  }
> >  static void unset_dirty_tracking(void)
> >@@ -611,10 +624,17 @@ static int block_save_setup(QEMUFile *f, void *opaque)
> >              block_mig_state.submitted, block_mig_state.transferred);
> >      qemu_mutex_lock_iothread();
> >-    init_blk_migration(f);
> >      /* start track dirty blocks */
> >-    set_dirty_tracking();
> >+    ret = set_dirty_tracking();
> >+
> >+    if (ret) {
> >+        qemu_mutex_unlock_iothread();
> >+        return ret;
> >+    }
> >+
> >+    init_blk_migration(f);
> >+
> >      qemu_mutex_unlock_iothread();
> >      ret = flush_blks(f);
> >diff --git a/block.c b/block.c
> >index acb70fd..93006de 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -5079,7 +5079,8 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, 
> >QEMUIOVector *qiov)
> >      return true;
> >  }
> >-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
> >granularity)
> >+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int 
> >granularity,
> >+                                          Error **errp)
> >  {
> >      int64_t bitmap_size;
> >      BdrvDirtyBitmap *bitmap;
> >@@ -5088,7 +5089,12 @@ BdrvDirtyBitmap 
> >*bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity)
> >      granularity >>= BDRV_SECTOR_BITS;
> >      assert(granularity);
> >-    bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
> >+    bitmap_size = bdrv_getlength(bs);
> >+    if (bitmap_size < 0) {
> >+        error_setg(errp, "could not get length of device");

Why not error_setg_errno()?

> >+        return NULL;
> >+    }
> >+    bitmap_size >>= BDRV_SECTOR_BITS;
> >      bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
> >      bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
> >      QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);

Kevin