Re: [Qemu-block] [PATCH v2 02/10] qcow2/bitmap: avoid adjusting bm->flags for RO bitmaps

[Vladimir Sementsov-Ogievskiy]

13.06.2018 05:06, John Snow wrote:
> Instead of always setting IN_USE, do this conditionally based on
> whether or not the bitmap is read-only. Eliminate the two-pass
> processing and move the second loop to the failure case.
>
> This will allow us to show the flags exactly as they appear
> on-disk for bitmaps in `qemu-img info`.
>
> Signed-off-by: John Snow <address@hidden>
> ---
>   block/qcow2-bitmap.c | 51 +++++++++++++++++++++------------------------------
>   1 file changed, 21 insertions(+), 30 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 0670e5eb41..85c1b5afe3 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -919,13 +919,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)
>   {
> @@ -944,8 +937,7 @@ 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 */
> @@ -968,35 +960,34 @@ 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);
>               }

+                bm->dirty_bitmap = bitmap; [1]

> -            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;
> +        }
> +    }
> +
>       bitmap_list_free(bm_list);
>   
> -    return header_updated;
> +    return needs_update;
>   
>   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) {

hm stop, at this point, you didn't yet set it. may be in later patches. 
I remember that you did it in previous version of patch series, so [1].

> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> +        }
> +    }
>       bitmap_list_free(bm_list);
>   
>       return false;

with [1]:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

--
Best regards,
Vladimir