[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] block: Handle error of bdrv_getlength in bdr
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v2] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap |
Date: |
Wed, 16 Apr 2014 09:16:41 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, 04/15 16:07, Max Reitz wrote:
> On 15.04.2014 05:15, Fam Zheng wrote:
> >bdrv_getlength could fail, check the return value before using it.
> >
> >Signed-off-by: Fam Zheng <address@hidden>
> >
> >---
> >v2: Make use of error_setg_errno and -errno. (Kevin)
> >
> >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..2a6df66 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 -errno;
>
> Where do you get this errno from? Some g_malloc() in
> bdrv_create_dirty_bitmap may set it, but I'd just return -EIO here. Probably
> the most likely reason for bdrv_create_dirty_bitmap() to fail is
> bdrv_getlength() having failed, and this will not set errno.
The error path in bdrv_create_dirty_bitmap sets errno with error_setg_errno(),
as you commented below.
I'll respin to make this clearer.
>
> > }
> > 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 990a754..e24b955 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -5096,7 +5096,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;
> >@@ -5105,7 +5106,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_errno(errp, bitmap_size, "could not get length of
> >device");
>
> This should be -bitmap_size.
Ah yes, my mistake.
Thanks,
Fam