Re: [Qemu-devel] [PATCH v2] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap

[Max Reitz]

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.

>   }
>   
>   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);