| Message ID | 20180512012537.22478-4-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-img: add bitmap queries | expand |
12.05.2018 04:25, John Snow wrote: > Instead of always setting IN_USE, handle whether or not the bitmap > is read-only instead of a two-loop pass. This will allow us to show > the flags exactly as they appear for bitmaps in `qemu-img info`. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/qcow2-bitmap.c | 48 ++++++++++++++++++++---------------------------- > 1 file changed, 20 insertions(+), 28 deletions(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index d89758f235..fc8d52fc3e 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -942,13 +942,6 @@ fail: > return ret; > } > > -/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ > -static void release_dirty_bitmap_helper(gpointer bitmap, > - gpointer bs) > -{ > - bdrv_release_dirty_bitmap(bs, bitmap); > -} > - > /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ > static void set_readonly_helper(gpointer bitmap, gpointer value) > { > @@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) > BDRVQcow2State *s = bs->opaque; > Qcow2BitmapList *bm_list; > Qcow2Bitmap *bm; > - GSList *created_dirty_bitmaps = NULL; > bool header_updated = false; > + bool needs_update = false; > > if (s->nb_bitmaps == 0) { > /* No bitmaps - nothing to do */ > @@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) > bdrv_disable_dirty_bitmap(bitmap); > } > bdrv_dirty_bitmap_set_persistance(bitmap, true); > - bm->flags |= BME_FLAG_IN_USE; > - created_dirty_bitmaps = > - g_slist_append(created_dirty_bitmaps, bitmap); > - } > - } > - > - if (created_dirty_bitmaps != NULL) { > - if (can_write(bs)) { > - /* in_use flags must be updated */ > - int ret = update_ext_header_and_dir_in_place(bs, bm_list); > - if (ret < 0) { > - error_setg_errno(errp, -ret, "Can't update bitmap directory"); > - goto fail; > + if (can_write(bs)) { > + bm->flags |= BME_FLAG_IN_USE; > + needs_update = true; > + } else { > + bdrv_dirty_bitmap_set_readonly(bitmap, true); oops. This shows, that patch 01 was incorrect: before this (03) patch, in this case we'll have IN_USE set in cache but not in the image. > } > - header_updated = true; > - } else { > - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, > - (gpointer)true); > } > } > > - g_slist_free(created_dirty_bitmaps); > + /* in_use flags must be updated */ > + if (needs_update) { > + int ret = update_ext_header_and_dir_in_place(bs, bm_list); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Can't update bitmap directory"); > + goto fail; > + } > + header_updated = true; > + } > return header_updated; may be safely changed to return needs_update, as if we failed to update, we'll go to fail. > > fail: > - g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs); > - g_slist_free(created_dirty_bitmaps); > + QSIMPLEQ_FOREACH(bm, bm_list, entry) { > + if (bm->dirty_bitmap) { > + bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap); > + } > + } > del_bitmap_list(bs); > > return false; overall looks good, may be worth move it before 01.
On 05/14/2018 08:44 AM, Vladimir Sementsov-Ogievskiy wrote: > 12.05.2018 04:25, John Snow wrote: >> Instead of always setting IN_USE, handle whether or not the bitmap >> is read-only instead of a two-loop pass. This will allow us to show >> the flags exactly as they appear for bitmaps in `qemu-img info`. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/qcow2-bitmap.c | 48 >> ++++++++++++++++++++---------------------------- >> 1 file changed, 20 insertions(+), 28 deletions(-) >> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index d89758f235..fc8d52fc3e 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c >> @@ -942,13 +942,6 @@ fail: >> return ret; >> } >> -/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ >> -static void release_dirty_bitmap_helper(gpointer bitmap, >> - gpointer bs) >> -{ >> - bdrv_release_dirty_bitmap(bs, bitmap); >> -} >> - >> /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ >> static void set_readonly_helper(gpointer bitmap, gpointer value) >> { >> @@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState >> *bs, Error **errp) >> BDRVQcow2State *s = bs->opaque; >> Qcow2BitmapList *bm_list; >> Qcow2Bitmap *bm; >> - GSList *created_dirty_bitmaps = NULL; >> bool header_updated = false; >> + bool needs_update = false; >> if (s->nb_bitmaps == 0) { >> /* No bitmaps - nothing to do */ >> @@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState >> *bs, Error **errp) >> bdrv_disable_dirty_bitmap(bitmap); >> } >> bdrv_dirty_bitmap_set_persistance(bitmap, true); >> - bm->flags |= BME_FLAG_IN_USE; >> - created_dirty_bitmaps = >> - g_slist_append(created_dirty_bitmaps, bitmap); >> - } >> - } >> - >> - if (created_dirty_bitmaps != NULL) { >> - if (can_write(bs)) { >> - /* in_use flags must be updated */ >> - int ret = update_ext_header_and_dir_in_place(bs, bm_list); >> - if (ret < 0) { >> - error_setg_errno(errp, -ret, "Can't update bitmap >> directory"); >> - goto fail; >> + if (can_write(bs)) { >> + bm->flags |= BME_FLAG_IN_USE; >> + needs_update = true; >> + } else { >> + bdrv_dirty_bitmap_set_readonly(bitmap, true); > > oops. This shows, that patch 01 was incorrect: before this (03) patch, > in this case we'll have IN_USE set in cache but not in the image. > Ah, I can try to order this patch first so that the cache stays semantically correct. >> } >> - header_updated = true; >> - } else { >> - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, >> - (gpointer)true); >> } >> } >> - g_slist_free(created_dirty_bitmaps); >> + /* in_use flags must be updated */ >> + if (needs_update) { >> + int ret = update_ext_header_and_dir_in_place(bs, bm_list); >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "Can't update bitmap >> directory"); >> + goto fail; >> + } >> + header_updated = true; >> + } >> return header_updated; > > may be safely changed to return needs_update, as if we failed to update, > we'll go to fail. > Ugh, yes. >> fail: >> - g_slist_foreach(created_dirty_bitmaps, >> release_dirty_bitmap_helper, bs); >> - g_slist_free(created_dirty_bitmaps); >> + QSIMPLEQ_FOREACH(bm, bm_list, entry) { >> + if (bm->dirty_bitmap) { >> + bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap); >> + } >> + } >> del_bitmap_list(bs); >> return false; > > overall looks good, may be worth move it before 01. >
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index d89758f235..fc8d52fc3e 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -942,13 +942,6 @@ fail: return ret; } -/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ -static void release_dirty_bitmap_helper(gpointer bitmap, - gpointer bs) -{ - bdrv_release_dirty_bitmap(bs, bitmap); -} - /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ static void set_readonly_helper(gpointer bitmap, gpointer value) { @@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) BDRVQcow2State *s = bs->opaque; Qcow2BitmapList *bm_list; Qcow2Bitmap *bm; - GSList *created_dirty_bitmaps = NULL; bool header_updated = false; + bool needs_update = false; if (s->nb_bitmaps == 0) { /* No bitmaps - nothing to do */ @@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) bdrv_disable_dirty_bitmap(bitmap); } bdrv_dirty_bitmap_set_persistance(bitmap, true); - bm->flags |= BME_FLAG_IN_USE; - created_dirty_bitmaps = - g_slist_append(created_dirty_bitmaps, bitmap); - } - } - - if (created_dirty_bitmaps != NULL) { - if (can_write(bs)) { - /* in_use flags must be updated */ - int ret = update_ext_header_and_dir_in_place(bs, bm_list); - if (ret < 0) { - error_setg_errno(errp, -ret, "Can't update bitmap directory"); - goto fail; + if (can_write(bs)) { + bm->flags |= BME_FLAG_IN_USE; + needs_update = true; + } else { + bdrv_dirty_bitmap_set_readonly(bitmap, true); } - header_updated = true; - } else { - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, - (gpointer)true); } } - g_slist_free(created_dirty_bitmaps); + /* in_use flags must be updated */ + if (needs_update) { + int ret = update_ext_header_and_dir_in_place(bs, bm_list); + if (ret < 0) { + error_setg_errno(errp, -ret, "Can't update bitmap directory"); + goto fail; + } + header_updated = true; + } return header_updated; fail: - g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs); - g_slist_free(created_dirty_bitmaps); + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + if (bm->dirty_bitmap) { + bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap); + } + } del_bitmap_list(bs); return false;
Instead of always setting IN_USE, handle whether or not the bitmap is read-only instead of a two-loop pass. This will allow us to show the flags exactly as they appear for bitmaps in `qemu-img info`. Signed-off-by: John Snow <jsnow@redhat.com> --- block/qcow2-bitmap.c | 48 ++++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 28 deletions(-)