| Message ID | 1397531754-11737-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <famz@redhat.com> > > --- > v2: Make use of error_setg_errno and -errno. (Kevin) > > 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..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. > } > > 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. Max > + 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 b3230a2..2b51eec 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -429,7 +429,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);
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 <famz@redhat.com> > > > >--- > >v2: Make use of error_setg_errno and -errno. (Kevin) > > > >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..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
On Wed, 04/16 09:16, Fam Zheng wrote: > 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 <famz@redhat.com> > > > > > >--- > > >v2: Make use of error_setg_errno and -errno. (Kevin) > > > > > >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..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(), Sorry, please ignore this, it's not set. I missed an "errno = -bitmap_size" there. Fam
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; } 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"); + 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 b3230a2..2b51eec 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -429,7 +429,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);