| Message ID | 1396418643-16850-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <famz@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 <mreitz@redhat.com> > } > > 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);
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 <famz@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 <mreitz@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
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; } 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);
bdrv_getlength could fail, check the return value before using it. Signed-off-by: Fam Zheng <famz@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(-)