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 <f...@redhat.com>
> >---
> > 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 <mre...@redhat.com>
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
