* [Qemu-devel] [PATCH] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap
@ 2014-04-02 6:04 Fam Zheng
2014-04-03 14:44 ` Max Reitz
0 siblings, 1 reply; 2+ messages in thread
From: Fam Zheng @ 2014-04-02 6:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Benoit Canet, Stefan Hajnoczi, Max Reitz
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;
}
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);
--
1.9.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap
2014-04-02 6:04 [Qemu-devel] [PATCH] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap Fam Zheng
@ 2014-04-03 14:44 ` Max Reitz
0 siblings, 0 replies; 2+ messages in thread
From: Max Reitz @ 2014-04-03 14:44 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Benoit Canet, Stefan Hajnoczi
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);
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-04-03 14:44 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-02 6:04 [Qemu-devel] [PATCH] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap Fam Zheng
2014-04-03 14:44 ` Max Reitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).