From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVisP-0000e7-9F for qemu-devel@nongnu.org; Thu, 03 Apr 2014 10:44:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WVisI-0007oz-VC for qemu-devel@nongnu.org; Thu, 03 Apr 2014 10:44:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33297) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVisI-0007oo-Nm for qemu-devel@nongnu.org; Thu, 03 Apr 2014 10:44:14 -0400 Message-ID: <533D73B4.1010609@redhat.com> Date: Thu, 03 Apr 2014 16:44:04 +0200 From: Max Reitz MIME-Version: 1.0 References: <1396418643-16850-1-git-send-email-famz@redhat.com> In-Reply-To: <1396418643-16850-1-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Benoit Canet , Stefan Hajnoczi 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 > --- > 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 > } > > 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"); > + 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); > diff --git a/block/mirror.c b/block/mirror.c > index 0ef41f9..2618c37 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -605,7 +605,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > s->granularity = granularity; > s->buf_size = MAX(buf_size, granularity); > > - s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity); > + s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, errp); > + if (!s->dirty_bitmap) { > + return; > + } > bdrv_set_enable_write_cache(s->target, true); > bdrv_set_on_error(s->target, on_target_error, on_target_error); > bdrv_iostatus_enable(s->target); > diff --git a/include/block/block.h b/include/block/block.h > index 1ed55d8..8e70a57 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -428,7 +428,8 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov); > > struct HBitmapIter; > typedef struct BdrvDirtyBitmap BdrvDirtyBitmap; > -BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity); > +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity, > + Error **errp); > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); > BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); > int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);