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

[Max Reitz <mreitz@redhat.com>]

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