[Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries

[Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries

[John Snow]

Allow qemu-img to show information about bitmaps stored in qcow2 images.
Add a 'bitmap' meta-command with 'dump' sub-command to retrieve a list of
dirty regions in bitmaps stored in a qcow2 image.

RFC:
- I am not 1000% convinced the bm_list caching is perfectly safe,
  especially with respect to migration and inactivation. There are
  more efficiencies and tweaks I can make, but I think this is the
  minimal set.

- I decided not to gate bitmap info in the "info" command behind
  extra flags.

- Bitmap data-gathering in "bitmap dump" could be made more
  space-efficient by just reporting one segment at a time,
  probably.

- `bitmap rm` and `bitmap mk` need extra work, so I am not submitting
  them just yet: rm needs more work around the remove persistence API,
  and make needs more work around the "can add" API.

- None of these commands will work with "in use" bitmaps; we need
  qemu-img check -r bitmaps support for this. I'm not sure what the
  right behavior to "fix" in-use bitmaps should be:
        - Cleared: This might be dangerous.
        - Fully Set: This is safer, but stupid.
        - Deleted: This might be the best option.
        - Un-set in-use: VERY dangerous; would rather not.

John Snow (12):
  qcow2-bitmap: cache bm_list
  qcow2/dirty-bitmap: cache loaded bitmaps
  block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
  qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
  qcow2-bitmap: track bitmap type
  qapi: add bitmap info
  qcow2-bitmap: add basic bitmaps info
  qjson: allow caller to ask for arbitrary indent
  qapi/block-core: add BitmapMapping and BitmapEntry structs
  qemu-img: split off common chunk of map command
  qemu-img: add bitmap dump
  qemu-img: add bitmap clear

 block/qcow2-bitmap.c     | 220 +++++++++++++++++--------
 block/qcow2.c            |   9 +
 block/qcow2.h            |   3 +
 include/qapi/qmp/qjson.h |   1 +
 qapi/block-core.json     |  92 ++++++++++-
 qemu-img-cmds.hx         |   6 +
 qemu-img.c               | 419 +++++++++++++++++++++++++++++++++++++----------
 qobject/qjson.c          |  21 +--
 8 files changed, 612 insertions(+), 159 deletions(-)

-- 
2.14.3

[Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list

[John Snow]

We don't need to re-read this list every time, exactly. We can keep it cached
and delete our copy when we flush to disk.

Because we don't try to flush bitmaps on close if there's nothing to flush,
add a new conditional to delete the state anyway for a clean exit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++--------------------
 block/qcow2.c        |  2 ++
 block/qcow2.h        |  2 ++
 3 files changed, 50 insertions(+), 28 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6e93ec43e1..fb0a4f3ec4 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
  * Get bitmap list from qcow2 image. Actually reads bitmap directory,
  * checks it and convert to bitmap list.
  */
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
-                                         uint64_t size, Error **errp)
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
 {
     int ret;
     BDRVQcow2State *s = bs->opaque;
@@ -545,6 +544,8 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
     Qcow2BitmapDirEntry *e;
     uint32_t nb_dir_entries = 0;
     Qcow2BitmapList *bm_list = NULL;
+    uint64_t offset = s->bitmap_directory_offset;
+    uint64_t size = s->bitmap_directory_size;
 
     if (size == 0) {
         error_setg(errp, "Requested bitmap directory size is zero");
@@ -636,6 +637,30 @@ fail:
     return NULL;
 }
 
+static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+
+    if (s->bitmap_list) {
+        return (Qcow2BitmapList *)s->bitmap_list;
+    }
+
+    bm_list = bitmap_list_load(bs, errp);
+    s->bitmap_list = bm_list;
+    return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    if (s->bitmap_list) {
+        bitmap_list_free(s->bitmap_list);
+        s->bitmap_list = NULL;
+    }
+}
+
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size)
@@ -656,8 +681,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
         return ret;
     }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, NULL);
+    bm_list = get_bitmap_list(bs, NULL);
     if (bm_list == NULL) {
         res->corruptions++;
         return -EINVAL;
@@ -707,8 +731,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     }
 
 out:
-    bitmap_list_free(bm_list);
-
     return ret;
 }
 
@@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         return false;
     }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
     if (bm_list == NULL) {
         return false;
     }
@@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     g_slist_free(created_dirty_bitmaps);
-    bitmap_list_free(bm_list);
-
     return header_updated;
 
 fail:
     g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
     g_slist_free(created_dirty_bitmaps);
-    bitmap_list_free(bm_list);
+    del_bitmap_list(bs);
 
     return false;
 }
@@ -1027,8 +1046,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
         return -EINVAL;
     }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
     if (bm_list == NULL) {
         return -EINVAL;
     }
@@ -1068,7 +1086,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
 
 out:
     g_slist_free(ro_dirty_bitmaps);
-    bitmap_list_free(bm_list);
 
     return ret;
 }
@@ -1277,8 +1294,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
         return;
     }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
     if (bm_list == NULL) {
         return;
     }
@@ -1300,7 +1316,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 
 fail:
     bitmap_free(bm);
-    bitmap_list_free(bm_list);
+}
+
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
+{
+    del_bitmap_list(bs);
 }
 
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
@@ -1317,12 +1337,12 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 
     if (!bdrv_has_changed_persistent_bitmaps(bs)) {
         /* nothing to do */
-        return;
+        goto out;
     }
 
     if (!can_write(bs)) {
         error_setg(errp, "No write access");
-        return;
+        goto out;
     }
 
     QSIMPLEQ_INIT(&drop_tables);
@@ -1330,10 +1350,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     if (s->nb_bitmaps == 0) {
         bm_list = bitmap_list_new();
     } else {
-        bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                                   s->bitmap_directory_size, errp);
+        bm_list = get_bitmap_list(bs, errp);
         if (bm_list == NULL) {
-            return;
+            goto out;
         }
     }
 
@@ -1414,8 +1433,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         g_free(tb);
     }
 
-    bitmap_list_free(bm_list);
-    return;
+    goto out;
 
 fail:
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1430,7 +1448,9 @@ fail:
         g_free(tb);
     }
 
-    bitmap_list_free(bm_list);
+ out:
+    del_bitmap_list(bs);
+    return;
 }
 
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
@@ -1495,14 +1515,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
         goto fail;
     }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
     if (bm_list == NULL) {
         goto fail;
     }
 
     found = find_bitmap_by_name(bm_list, name);
-    bitmap_list_free(bm_list);
     if (found) {
         error_setg(errp, "Bitmap with the same name is already stored");
         goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 2f36e632f9..7ae9000656 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
 
     if (!(s->flags & BDRV_O_INACTIVE)) {
         qcow2_inactivate(bs);
+    } else {
+        qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
     }
 
     cache_clean_timer_del(bs);
diff --git a/block/qcow2.h b/block/qcow2.h
index adf5c3950f..796a8c914b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
     uint64_t bitmap_directory_size;
     uint64_t bitmap_directory_offset;
     bool dirty_bitmaps_loaded;
+    void *bitmap_list;
 
     int flags;
     int qcow_version;
@@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                  Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list

[Vladimir Sementsov-Ogievskiy]

12.05.2018 04:25, John Snow wrote:
> We don't need to re-read this list every time, exactly. We can keep it cached
> and delete our copy when we flush to disk.

Why not simply delete cache only on close (unconditionally)? Why do we 
need to remove it after flush?

Actually, I think we need to remove it only in qcow2_inactive, after 
storing persistent bitmaps.


>
> Because we don't try to flush bitmaps on close if there's nothing to flush,
> add a new conditional to delete the state anyway for a clean exit.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++--------------------
>   block/qcow2.c        |  2 ++
>   block/qcow2.h        |  2 ++
>   3 files changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 6e93ec43e1..fb0a4f3ec4 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
>    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>    * checks it and convert to bitmap list.
>    */
> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
> -                                         uint64_t size, Error **errp)
> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
>   {
>       int ret;
>       BDRVQcow2State *s = bs->opaque;
> @@ -545,6 +544,8 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>       Qcow2BitmapDirEntry *e;
>       uint32_t nb_dir_entries = 0;
>       Qcow2BitmapList *bm_list = NULL;
> +    uint64_t offset = s->bitmap_directory_offset;
> +    uint64_t size = s->bitmap_directory_size;

Worth split this change as a refactoring patch (just remove extra 
parameters)?

>   
>       if (size == 0) {
>           error_setg(errp, "Requested bitmap directory size is zero");
> @@ -636,6 +637,30 @@ fail:
>       return NULL;
>   }
>   
> +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    Qcow2BitmapList *bm_list;
> +
> +    if (s->bitmap_list) {
> +        return (Qcow2BitmapList *)s->bitmap_list;
> +    }
> +
> +    bm_list = bitmap_list_load(bs, errp);
> +    s->bitmap_list = bm_list;
> +    return bm_list;
> +}
> +
> +static void del_bitmap_list(BlockDriverState *bs)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +
> +    if (s->bitmap_list) {
> +        bitmap_list_free(s->bitmap_list);
> +        s->bitmap_list = NULL;
> +    }
> +}
> +
>   int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>                                     void **refcount_table,
>                                     int64_t *refcount_table_size)
> @@ -656,8 +681,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>           return ret;
>       }
>   
> -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, NULL);
> +    bm_list = get_bitmap_list(bs, NULL);
>       if (bm_list == NULL) {
>           res->corruptions++;
>           return -EINVAL;
> @@ -707,8 +731,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>       }
>   
>   out:
> -    bitmap_list_free(bm_list);
> -
>       return ret;
>   }
>   
> @@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           return false;
>       }
>   
> -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +    bm_list = get_bitmap_list(bs, errp);
>       if (bm_list == NULL) {
>           return false;
>       }
> @@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       }
>   
>       g_slist_free(created_dirty_bitmaps);
> -    bitmap_list_free(bm_list);
> -
>       return header_updated;
>   
>   fail:
>       g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
>       g_slist_free(created_dirty_bitmaps);
> -    bitmap_list_free(bm_list);
> +    del_bitmap_list(bs);
>   
>       return false;
>   }
> @@ -1027,8 +1046,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>           return -EINVAL;
>       }
>   
> -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +    bm_list = get_bitmap_list(bs, errp);
>       if (bm_list == NULL) {
>           return -EINVAL;
>       }
> @@ -1068,7 +1086,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>   
>   out:
>       g_slist_free(ro_dirty_bitmaps);
> -    bitmap_list_free(bm_list);
>   
>       return ret;
>   }
> @@ -1277,8 +1294,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>           return;
>       }
>   
> -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +    bm_list = get_bitmap_list(bs, errp);
>       if (bm_list == NULL) {
>           return;
>       }
> @@ -1300,7 +1316,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>   
>   fail:
>       bitmap_free(bm);
> -    bitmap_list_free(bm_list);
> +}
> +
> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
> +{
> +    del_bitmap_list(bs);
>   }
>   
>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> @@ -1317,12 +1337,12 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>   
>       if (!bdrv_has_changed_persistent_bitmaps(bs)) {
>           /* nothing to do */
> -        return;
> +        goto out;
>       }
>   
>       if (!can_write(bs)) {
>           error_setg(errp, "No write access");
> -        return;
> +        goto out;
>       }
>   
>       QSIMPLEQ_INIT(&drop_tables);
> @@ -1330,10 +1350,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       if (s->nb_bitmaps == 0) {
>           bm_list = bitmap_list_new();
>       } else {
> -        bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                                   s->bitmap_directory_size, errp);
> +        bm_list = get_bitmap_list(bs, errp);
>           if (bm_list == NULL) {
> -            return;
> +            goto out;
>           }
>       }
>   
> @@ -1414,8 +1433,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           g_free(tb);
>       }
>   
> -    bitmap_list_free(bm_list);
> -    return;
> +    goto out;
>   
>   fail:
>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> @@ -1430,7 +1448,9 @@ fail:
>           g_free(tb);
>       }
>   
> -    bitmap_list_free(bm_list);
> + out:
> +    del_bitmap_list(bs);
> +    return;
>   }
>   
>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
> @@ -1495,14 +1515,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>           goto fail;
>       }
>   
> -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +    bm_list = get_bitmap_list(bs, errp);
>       if (bm_list == NULL) {
>           goto fail;
>       }
>   
>       found = find_bitmap_by_name(bm_list, name);
> -    bitmap_list_free(bm_list);
>       if (found) {
>           error_setg(errp, "Bitmap with the same name is already stored");
>           goto fail;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2f36e632f9..7ae9000656 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
>   
>       if (!(s->flags & BDRV_O_INACTIVE)) {
>           qcow2_inactivate(bs);
> +    } else {
> +        qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
>       }
>   
>       cache_clean_timer_del(bs);
> diff --git a/block/qcow2.h b/block/qcow2.h
> index adf5c3950f..796a8c914b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
>       uint64_t bitmap_directory_size;
>       uint64_t bitmap_directory_offset;
>       bool dirty_bitmaps_loaded;
> +    void *bitmap_list;
>   
>       int flags;
>       int qcow_version;
> @@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>                                    Error **errp);
>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs);
>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>   bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list

[Vladimir Sementsov-Ogievskiy]

14.05.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> We don't need to re-read this list every time, exactly. We can keep 
>> it cached
>> and delete our copy when we flush to disk.
>
> Why not simply delete cache only on close (unconditionally)? Why do we 
> need to remove it after flush?
>
> Actually, I think we need to remove it only in qcow2_inactive, after 
> storing persistent bitmaps.
>
>
>>
>> Because we don't try to flush bitmaps on close if there's nothing to 
>> flush,
>> add a new conditional to delete the state anyway for a clean exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 74 
>> ++++++++++++++++++++++++++++++++--------------------
>>   block/qcow2.c        |  2 ++
>>   block/qcow2.h        |  2 ++
>>   3 files changed, 50 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 6e93ec43e1..fb0a4f3ec4 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList 
>> *bm_list)
>>    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>>    * checks it and convert to bitmap list.
>>    */
>> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, 
>> uint64_t offset,
>> -                                         uint64_t size, Error **errp)
>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error 
>> **errp)
>>   {
>>       int ret;
>>       BDRVQcow2State *s = bs->opaque;
>> @@ -545,6 +544,8 @@ static Qcow2BitmapList 
>> *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>>       Qcow2BitmapDirEntry *e;
>>       uint32_t nb_dir_entries = 0;
>>       Qcow2BitmapList *bm_list = NULL;
>> +    uint64_t offset = s->bitmap_directory_offset;
>> +    uint64_t size = s->bitmap_directory_size;
>
> Worth split this change as a refactoring patch (just remove extra 
> parameters)?
>
>>         if (size == 0) {
>>           error_setg(errp, "Requested bitmap directory size is zero");
>> @@ -636,6 +637,30 @@ fail:
>>       return NULL;
>>   }
>>   +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, 
>> Error **errp)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    Qcow2BitmapList *bm_list;
>> +
>> +    if (s->bitmap_list) {
>> +        return (Qcow2BitmapList *)s->bitmap_list;
>> +    }
>> +
>> +    bm_list = bitmap_list_load(bs, errp);
>> +    s->bitmap_list = bm_list;
>> +    return bm_list;
>> +}
>> +
>> +static void del_bitmap_list(BlockDriverState *bs)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +
>> +    if (s->bitmap_list) {
>> +        bitmap_list_free(s->bitmap_list);
>> +        s->bitmap_list = NULL;
>> +    }
>> +}

so, with this functions, we see, that list is always safely loaded 
through the cache. But we need also guarantee, that list is always saved 
through the cache. There are a lot of functions, which stores an 
abstract bitmap list, given as a parameter, but we want always store our 
cache..

>> +
>>   int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
>> BdrvCheckResult *res,
>>                                     void **refcount_table,
>>                                     int64_t *refcount_table_size)
>> @@ -656,8 +681,7 @@ int 
>> qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult 
>> *res,
>>           return ret;
>>       }
>>   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, NULL);
>> +    bm_list = get_bitmap_list(bs, NULL);
>>       if (bm_list == NULL) {
>>           res->corruptions++;
>>           return -EINVAL;
>> @@ -707,8 +731,6 @@ int 
>> qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult 
>> *res,
>>       }
>>     out:
>> -    bitmap_list_free(bm_list);
>> -
>>       return ret;
>>   }
>>   @@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState 
>> *bs, Error **errp)
>>           return false;
>>       }
>>   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return false;
>>       }
>> @@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState 
>> *bs, Error **errp)
>>       }
>>         g_slist_free(created_dirty_bitmaps);
>> -    bitmap_list_free(bm_list);
>> -
>>       return header_updated;
>>     fail:
>>       g_slist_foreach(created_dirty_bitmaps, 
>> release_dirty_bitmap_helper, bs);
>>       g_slist_free(created_dirty_bitmaps);
>> -    bitmap_list_free(bm_list);
>> +    del_bitmap_list(bs);
>>         return false;
>>   }
>> @@ -1027,8 +1046,7 @@ int 
>> qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>           return -EINVAL;
>>       }
>>   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return -EINVAL;
>>       }
>> @@ -1068,7 +1086,6 @@ int 
>> qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>     out:
>>       g_slist_free(ro_dirty_bitmaps);
>> -    bitmap_list_free(bm_list);
>>         return ret;
>>   }
>> @@ -1277,8 +1294,7 @@ void 
>> qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>           return;
>>       }
>>   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return;
>>       }
>> @@ -1300,7 +1316,11 @@ void 
>> qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>     fail:
>>       bitmap_free(bm);
>> -    bitmap_list_free(bm_list);
>> +}
>> +
>> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
>> +{
>> +    del_bitmap_list(bs);
>>   }
>>     void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, 
>> Error **errp)
>> @@ -1317,12 +1337,12 @@ void 
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>         if (!bdrv_has_changed_persistent_bitmaps(bs)) {
>>           /* nothing to do */
>> -        return;
>> +        goto out;
>>       }
>>         if (!can_write(bs)) {
>>           error_setg(errp, "No write access");
>> -        return;
>> +        goto out;
>>       }
>>         QSIMPLEQ_INIT(&drop_tables);
>> @@ -1330,10 +1350,9 @@ void 
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>       if (s->nb_bitmaps == 0) {
>>           bm_list = bitmap_list_new();
>>       } else {
>> -        bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                                   s->bitmap_directory_size, errp);
>> +        bm_list = get_bitmap_list(bs, errp);
>>           if (bm_list == NULL) {
>> -            return;
>> +            goto out;
>>           }
>>       }
>>   @@ -1414,8 +1433,7 @@ void 
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           g_free(tb);
>>       }
>>   -    bitmap_list_free(bm_list);
>> -    return;
>> +    goto out;
>>     fail:
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> @@ -1430,7 +1448,9 @@ fail:
>>           g_free(tb);
>>       }
>>   -    bitmap_list_free(bm_list);
>> + out:
>> +    del_bitmap_list(bs);
>> +    return;
>>   }
>>     int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>> @@ -1495,14 +1515,12 @@ bool 
>> qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>           goto fail;
>>       }
>>   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           goto fail;
>>       }
>>         found = find_bitmap_by_name(bm_list, name);
>> -    bitmap_list_free(bm_list);
>>       if (found) {
>>           error_setg(errp, "Bitmap with the same name is already 
>> stored");
>>           goto fail;
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 2f36e632f9..7ae9000656 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
>>         if (!(s->flags & BDRV_O_INACTIVE)) {
>>           qcow2_inactivate(bs);
>> +    } else {
>> +        qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
>>       }
>>         cache_clean_timer_del(bs);
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index adf5c3950f..796a8c914b 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
>>       uint64_t bitmap_directory_size;
>>       uint64_t bitmap_directory_offset;
>>       bool dirty_bitmaps_loaded;
>> +    void *bitmap_list;
>>         int flags;
>>       int qcow_version;
>> @@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState 
>> *bs, Error **errp);
>>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool 
>> *header_updated,
>>                                    Error **errp);
>>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState 
>> *bs);
>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, 
>> Error **errp);
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>   bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list

[John Snow]

On 05/14/2018 08:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
>> 12.05.2018 04:25, John Snow wrote:
>>> We don't need to re-read this list every time, exactly. We can keep
>>> it cached
>>> and delete our copy when we flush to disk.
>>
>> Why not simply delete cache only on close (unconditionally)? Why do we
>> need to remove it after flush?
>>
>> Actually, I think we need to remove it only in qcow2_inactive, after
>> storing persistent bitmaps.
>>
>>
>>>
>>> Because we don't try to flush bitmaps on close if there's nothing to
>>> flush,
>>> add a new conditional to delete the state anyway for a clean exit.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   block/qcow2-bitmap.c | 74
>>> ++++++++++++++++++++++++++++++++--------------------
>>>   block/qcow2.c        |  2 ++
>>>   block/qcow2.h        |  2 ++
>>>   3 files changed, 50 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 6e93ec43e1..fb0a4f3ec4 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
>>> *bm_list)
>>>    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>>>    * checks it and convert to bitmap list.
>>>    */
>>> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>>> uint64_t offset,
>>> -                                         uint64_t size, Error **errp)
>>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
>>> **errp)
>>>   {
>>>       int ret;
>>>       BDRVQcow2State *s = bs->opaque;
>>> @@ -545,6 +544,8 @@ static Qcow2BitmapList
>>> *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>>>       Qcow2BitmapDirEntry *e;
>>>       uint32_t nb_dir_entries = 0;
>>>       Qcow2BitmapList *bm_list = NULL;
>>> +    uint64_t offset = s->bitmap_directory_offset;
>>> +    uint64_t size = s->bitmap_directory_size;
>>
>> Worth split this change as a refactoring patch (just remove extra
>> parameters)?
>>
>>>         if (size == 0) {
>>>           error_setg(errp, "Requested bitmap directory size is zero");
>>> @@ -636,6 +637,30 @@ fail:
>>>       return NULL;
>>>   }
>>>   +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs,
>>> Error **errp)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    Qcow2BitmapList *bm_list;
>>> +
>>> +    if (s->bitmap_list) {
>>> +        return (Qcow2BitmapList *)s->bitmap_list;
>>> +    }
>>> +
>>> +    bm_list = bitmap_list_load(bs, errp);
>>> +    s->bitmap_list = bm_list;
>>> +    return bm_list;
>>> +}
>>> +
>>> +static void del_bitmap_list(BlockDriverState *bs)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +
>>> +    if (s->bitmap_list) {
>>> +        bitmap_list_free(s->bitmap_list);
>>> +        s->bitmap_list = NULL;
>>> +    }
>>> +}
> 
> so, with this functions, we see, that list is always safely loaded
> through the cache. But we need also guarantee, that list is always saved
> through the cache. There are a lot of functions, which stores an
> abstract bitmap list, given as a parameter, but we want always store our
> cache..
> 

Do you have an example?

These functions return a bitmap list:

bitmap_list_new()
bitmap_list_load(...);
get_bitmap_list(...);


bitmap_list_new creates a new bitmap_list, it's called by
bitmap_list_load and qcow2_store_persistent_dirty_bitmaps.

qcow2_store_persistent_dirty_bitmaps doesn't matter right now because we
drop the cache by the end of that function.

bitmap_list_load is only called by get_bitmap_list now, and
get_bitmap_list caches that bitmap list as s->bitmap_list.


I think any other function that takes a bitmap list must be getting it
from a location where it's cached -- except in store, which trashes it
anyway.

What I can do here is to make get_bitmap_list create a new bitmap list
if s->nb_bitmaps is zero, and remove the new call in store -- then we
can be really very confident that any bitmap list getting passed around
is definitely the cached one.

Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list

[John Snow]

On 05/14/2018 07:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> We don't need to re-read this list every time, exactly. We can keep it
>> cached
>> and delete our copy when we flush to disk.
> 
> Why not simply delete cache only on close (unconditionally)? Why do we
> need to remove it after flush?
> 

I was being imprecise with my language again -- I'm doing it on
qcow2_inactivate, through qcow2_store_persistent_dirty_bitmaps.

I technically don't even need to destroy the cache *then*, I could just
do it unconditionally in qcow2_close.

I just felt it was "safer" to drop the cache after a store as a natural
point of synchronization, but it *should* be fine without.

> Actually, I think we need to remove it only in qcow2_inactive, after
> storing persistent bitmaps.
> 

So would you prefer the unconditional drop on close, or a drop for every
inactivate? You still need a drop on close in that case, because we may
not call inactivate on close, and I am still trying to build the cache
for read only images, so to prevent leaks I need to clean that up.

> 
>>
>> Because we don't try to flush bitmaps on close if there's nothing to
>> flush,
>> add a new conditional to delete the state anyway for a clean exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 74
>> ++++++++++++++++++++++++++++++++--------------------
>>   block/qcow2.c        |  2 ++
>>   block/qcow2.h        |  2 ++
>>   3 files changed, 50 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 6e93ec43e1..fb0a4f3ec4 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
>> *bm_list)
>>    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>>    * checks it and convert to bitmap list.
>>    */
>> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>> uint64_t offset,
>> -                                         uint64_t size, Error **errp)
>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
>> **errp)
>>   {
>>       int ret;
>>       BDRVQcow2State *s = bs->opaque;
>> @@ -545,6 +544,8 @@ static Qcow2BitmapList
>> *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>>       Qcow2BitmapDirEntry *e;
>>       uint32_t nb_dir_entries = 0;
>>       Qcow2BitmapList *bm_list = NULL;
>> +    uint64_t offset = s->bitmap_directory_offset;
>> +    uint64_t size = s->bitmap_directory_size;
> 
> Worth split this change as a refactoring patch (just remove extra
> parameters)?
> 

Sure, this patch looks a little long.

[Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps

[John Snow]

For bitmaps that we succeeded in loading, we can cache a reference
to that object. This will let us iterate over the more convenient
form of in-memory bitmaps for qemu-img bitmap manipulation tools.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fb0a4f3ec4..d89758f235 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -986,6 +986,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             if (bitmap == NULL) {
                 goto fail;
             }
+            bm->dirty_bitmap = bitmap;
 
             if (!(bm->flags & BME_FLAG_AUTO)) {
                 bdrv_disable_dirty_bitmap(bitmap);
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps

[Vladimir Sementsov-Ogievskiy]

12.05.2018 04:25, John Snow wrote:
> For bitmaps that we succeeded in loading, we can cache a reference
> to that object. This will let us iterate over the more convenient
> form of in-memory bitmaps for qemu-img bitmap manipulation tools.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index fb0a4f3ec4..d89758f235 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -986,6 +986,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>               if (bitmap == NULL) {
>                   goto fail;
>               }
> +            bm->dirty_bitmap = bitmap;
>   
>               if (!(bm->flags & BME_FLAG_AUTO)) {
>                   bdrv_disable_dirty_bitmap(bitmap);

Looks correct, I just suppose to reuse this information somehow in 
qcow2_store_persistent_dirty_bitmaps, at least for an assert.

-- 
Best regards,
Vladimir

[Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps

[John Snow]

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

Re: [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps

[Vladimir Sementsov-Ogievskiy]

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.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps

[John Snow]

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.
>

[Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO

[John Snow]

For the purposes of qemu-img manipulation and querying of bitmaps, load
bitmaps that are "in use" -- if the image is read only. This will allow
us to diagnose problems with this flag using the qemu-img tool.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fc8d52fc3e..b556dbdccd 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -346,7 +346,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
     uint32_t granularity;
     BdrvDirtyBitmap *bitmap = NULL;
 
-    if (bm->flags & BME_FLAG_IN_USE) {
+    if (can_write(bs) && (bm->flags & BME_FLAG_IN_USE)) {
         error_setg(errp, "Bitmap '%s' is in use", bm->name);
         goto fail;
     }
@@ -974,23 +974,21 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (!(bm->flags & BME_FLAG_IN_USE)) {
-            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
-            if (bitmap == NULL) {
-                goto fail;
-            }
-            bm->dirty_bitmap = bitmap;
+        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+        if (bitmap == NULL) {
+            goto fail;
+        }
+        bm->dirty_bitmap = bitmap;
 
-            if (!(bm->flags & BME_FLAG_AUTO)) {
-                bdrv_disable_dirty_bitmap(bitmap);
-            }
-            bdrv_dirty_bitmap_set_persistance(bitmap, true);
-            if (can_write(bs)) {
-                bm->flags |= BME_FLAG_IN_USE;
-                needs_update = true;
-            } else {
-                bdrv_dirty_bitmap_set_readonly(bitmap, true);
-            }
+        if (!(bm->flags & BME_FLAG_AUTO)) {
+            bdrv_disable_dirty_bitmap(bitmap);
+        }
+        bdrv_dirty_bitmap_set_persistance(bitmap, true);
+        if (can_write(bs)) {
+            bm->flags |= BME_FLAG_IN_USE;
+            needs_update = true;
+        } else {
+            bdrv_dirty_bitmap_set_readonly(bitmap, true);
         }
     }
 
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO

[Vladimir Sementsov-Ogievskiy]

12.05.2018 04:25, John Snow wrote:
> For the purposes of qemu-img manipulation and querying of bitmaps, load
> bitmaps that are "in use" -- if the image is read only. This will allow
> us to diagnose problems with this flag using the qemu-img tool.

It looks unsafe.. We can load them, then reopen rw and then store them 
on vm close, the result would be unpredictable. And they become 
available for qmp commands..

If we want to load them, we may need some additional state for such 
dirty bitmaps, something like "invalid", to be sure, that they will not 
be used like normal bitmap. And we may
need some additional flag to load or not load them, and this behavior 
will be unrelated to can_write().

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 32 +++++++++++++++-----------------
>   1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index fc8d52fc3e..b556dbdccd 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -346,7 +346,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>       uint32_t granularity;
>       BdrvDirtyBitmap *bitmap = NULL;
>   
> -    if (bm->flags & BME_FLAG_IN_USE) {
> +    if (can_write(bs) && (bm->flags & BME_FLAG_IN_USE)) {
>           error_setg(errp, "Bitmap '%s' is in use", bm->name);
>           goto fail;
>       }
> @@ -974,23 +974,21 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       }
>   
>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
> -            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> -            if (bitmap == NULL) {
> -                goto fail;
> -            }
> -            bm->dirty_bitmap = bitmap;
> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> +        if (bitmap == NULL) {
> +            goto fail;
> +        }
> +        bm->dirty_bitmap = bitmap;
>   
> -            if (!(bm->flags & BME_FLAG_AUTO)) {
> -                bdrv_disable_dirty_bitmap(bitmap);
> -            }
> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
> -            if (can_write(bs)) {
> -                bm->flags |= BME_FLAG_IN_USE;
> -                needs_update = true;
> -            } else {
> -                bdrv_dirty_bitmap_set_readonly(bitmap, true);
> -            }
> +        if (!(bm->flags & BME_FLAG_AUTO)) {
> +            bdrv_disable_dirty_bitmap(bitmap);
> +        }
> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
> +        if (can_write(bs)) {
> +            bm->flags |= BME_FLAG_IN_USE;
> +            needs_update = true;
> +        } else {
> +            bdrv_dirty_bitmap_set_readonly(bitmap, true);
>           }
>       }
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO

[John Snow]

On 05/14/2018 08:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> For the purposes of qemu-img manipulation and querying of bitmaps, load
>> bitmaps that are "in use" -- if the image is read only. This will allow
>> us to diagnose problems with this flag using the qemu-img tool.
> 
> It looks unsafe.. We can load them, then reopen rw and then store them
> on vm close, the result would be unpredictable. And they become
> available for qmp commands..
> 
> If we want to load them, we may need some additional state for such
> dirty bitmaps, something like "invalid", to be sure, that they will not
> be used like normal bitmap. And we may
> need some additional flag to load or not load them, and this behavior
> will be unrelated to can_write().
> 

Yeah, you're right -- reopening is scary. I'll see what I can do.

The goal is just to get them in memory so that qemu-img can use the
standard BdrvDirtyBitmap API to print various info about them for the user.

Since 'info' and 'dump' don't need RW access, it'd be nicest for these
commands to work in RO mode, hence the patch.

[Qemu-devel] [RFC PATCH 05/12] qcow2-bitmap: track bitmap type

[John Snow]

We only have one type of persistent bitmap right now, but I'd like the
qemu-img tool to be able to give good diagnostic information if it sees
an unknown/unsupported type.

We do enforce it to be the dirty tracking type in check_dir_entry, but
I wanted positive affirmation of the type in the forthcoming info script.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b556dbdccd..60e01abfd7 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -83,6 +83,7 @@ typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
 typedef struct Qcow2Bitmap {
     Qcow2BitmapTable table;
     uint32_t flags;
+    uint8_t type;
     uint8_t granularity_bits;
     char *name;
 
@@ -608,6 +609,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
         bm->table.offset = e->bitmap_table_offset;
         bm->table.size = e->bitmap_table_size;
         bm->flags = e->flags;
+        bm->type = e->type;
         bm->granularity_bits = e->granularity_bits;
         bm->name = dir_entry_copy_name(e);
         QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
-- 
2.14.3

[Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info

[John Snow]

Add some of the necessary scaffolding for reporting bitmap information.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c50517bff3..8f33f41ce7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -33,6 +33,61 @@
             'date-sec': 'int', 'date-nsec': 'int',
             'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
 
+##
+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+#                  segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
+
+##
+# @BitmapFlagEnum:
+#
+# An enumeration of possible flags for serialized bitmaps.
+#
+# @in-use: This bitmap is considered to be in-use, and may now be inconsistent.
+#
+# @auto: This bitmap must reflect any and all changes to the file it describes.
+#        The type of this bitmap must be @DirtyTrackingBitmap.
+#
+# @extra-data-compatible: This bitmap has extra information associated with it.
+#
+# @unknown: This bitmap has unknown or reserved properties.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+                                      'extra-data-compatible', 'unknown' ] }
+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#
+# @count: Overall bitmap dirtiness, in bytes.
+#
+# @flags: Bitmap flags, if any.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapInfo',
+  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity': 'int',
+            'count': 'int', '*flags': ['BitmapFlagEnum']
+  }
+}
+
 ##
 # @ImageInfoSpecificQCow2EncryptionBase:
 #
@@ -69,6 +124,8 @@
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @bitmaps: list of image bitmaps (since 2.13)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +134,8 @@
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['BitmapInfo']
   } }
 
 ##
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info

[Vladimir Sementsov-Ogievskiy]

12.05.2018 04:25, John Snow wrote:
> Add some of the necessary scaffolding for reporting bitmap information.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   qapi/block-core.json | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c50517bff3..8f33f41ce7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -33,6 +33,61 @@
>               'date-sec': 'int', 'date-nsec': 'int',
>               'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>   
> +##
> +# @BitmapTypeEnum:
> +#
> +# An enumeration of possible serialized bitmap types.
> +#
> +# @dirty-tracking: This bitmap records information on dirty
> +#                  segments within the file.
> +#
> +# @unknown: This bitmap is of an unknown or reserved type.
> +#
> +# Since: 2.13
> +##
> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
> +
> +##
> +# @BitmapFlagEnum:
> +#
> +# An enumeration of possible flags for serialized bitmaps.
> +#
> +# @in-use: This bitmap is considered to be in-use, and may now be inconsistent.
> +#
> +# @auto: This bitmap must reflect any and all changes to the file it describes.
> +#        The type of this bitmap must be @DirtyTrackingBitmap.

logical, but I don't see this restriction in the spec. May be we need to 
update the spec

> +#
> +# @extra-data-compatible: This bitmap has extra information associated with it.

no, this flag means, that extra data is compatible. So, if you don't 
know what is this extra data, you can read and modify the bitmap, 
leaving this data as is. If this flag is unset, and there are some extra 
data, bitmap must not be used.

Finally, this spec should be consistent (or, may be better, duplicate) 
spec from docs/interop/qcow2.txt..
> +#
> +# @unknown: This bitmap has unknown or reserved properties.

Better is only "reserved flags" (not unknown and not properties), they 
are reserved by spec.

> +#
> +# Since: 2.13
> +##
> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
> +                                      'extra-data-compatible', 'unknown' ] }
> +
> +##
> +# @BitmapInfo:
> +#
> +# @name: The name of the bitmap.
> +#
> +# @type: The type of bitmap.
> +#
> +# @granularity: Bitmap granularity, in bytes.
> +#
> +# @count: Overall bitmap dirtiness, in bytes.
> +#
> +# @flags: Bitmap flags, if any.
> +#
> +# Since: 2.13
> +#
> +##
> +{ 'struct': 'BitmapInfo',
> +  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity': 'int',
> +            'count': 'int', '*flags': ['BitmapFlagEnum']

may be worth add 'has-extra-data'

> +  }
> +}
> +
>   ##
>   # @ImageInfoSpecificQCow2EncryptionBase:
>   #
> @@ -69,6 +124,8 @@
>   # @encrypt: details about encryption parameters; only set if image
>   #           is encrypted (since 2.10)
>   #
> +# @bitmaps: list of image bitmaps (since 2.13)
> +#
>   # Since: 1.7
>   ##
>   { 'struct': 'ImageInfoSpecificQCow2',
> @@ -77,7 +134,8 @@
>         '*lazy-refcounts': 'bool',
>         '*corrupt': 'bool',
>         'refcount-bits': 'int',
> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> +      '*bitmaps': ['BitmapInfo']
>     } }
>   
>   ##


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info

[John Snow]

On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add some of the necessary scaffolding for reporting bitmap information.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   qapi/block-core.json | 60
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index c50517bff3..8f33f41ce7 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -33,6 +33,61 @@
>>               'date-sec': 'int', 'date-nsec': 'int',
>>               'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>>   +##
>> +# @BitmapTypeEnum:
>> +#
>> +# An enumeration of possible serialized bitmap types.
>> +#
>> +# @dirty-tracking: This bitmap records information on dirty
>> +#                  segments within the file.
>> +#
>> +# @unknown: This bitmap is of an unknown or reserved type.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>> +
>> +##
>> +# @BitmapFlagEnum:
>> +#
>> +# An enumeration of possible flags for serialized bitmaps.
>> +#
>> +# @in-use: This bitmap is considered to be in-use, and may now be
>> inconsistent.
>> +#
>> +# @auto: This bitmap must reflect any and all changes to the file it
>> describes.
>> +#        The type of this bitmap must be @DirtyTrackingBitmap.
> 
> logical, but I don't see this restriction in the spec. May be we need to
> update the spec
> 

Where did I even get this idea?

>> +#
>> +# @extra-data-compatible: This bitmap has extra information
>> associated with it.
> 
> no, this flag means, that extra data is compatible. So, if you don't
> know what is this extra data, you can read and modify the bitmap,
> leaving this data as is. If this flag is unset, and there are some extra
> data, bitmap must not be used.
> 

Oh, understood! Thank you for clearing that up.

> Finally, this spec should be consistent (or, may be better, duplicate)
> spec from docs/interop/qcow2.txt..

*cough* I guess if I get enough things wrong it's fair to say "RTFM!"

>> +#
>> +# @unknown: This bitmap has unknown or reserved properties.
> 
> Better is only "reserved flags" (not unknown and not properties), they
> are reserved by spec.
> 

Yeah, I'll take that stance.

>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
>> +                                      'extra-data-compatible',
>> 'unknown' ] }
>> +
>> +##
>> +# @BitmapInfo:
>> +#
>> +# @name: The name of the bitmap.
>> +#
>> +# @type: The type of bitmap.
>> +#
>> +# @granularity: Bitmap granularity, in bytes.
>> +#
>> +# @count: Overall bitmap dirtiness, in bytes.
>> +#
>> +# @flags: Bitmap flags, if any.
>> +#
>> +# Since: 2.13
>> +#
>> +##
>> +{ 'struct': 'BitmapInfo',
>> +  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':
>> 'int',
>> +            'count': 'int', '*flags': ['BitmapFlagEnum']
> 
> may be worth add 'has-extra-data'
> 

Since you pointed out the difference, I agree.

>> +  }
>> +}
>> +
>>   ##
>>   # @ImageInfoSpecificQCow2EncryptionBase:
>>   #
>> @@ -69,6 +124,8 @@
>>   # @encrypt: details about encryption parameters; only set if image
>>   #           is encrypted (since 2.10)
>>   #
>> +# @bitmaps: list of image bitmaps (since 2.13)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +134,8 @@
>>         '*lazy-refcounts': 'bool',
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> +      '*bitmaps': ['BitmapInfo']
>>     } }
>>     ##
> 
> 

Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info

[John Snow]

On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add some of the necessary scaffolding for reporting bitmap information.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   qapi/block-core.json | 60
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index c50517bff3..8f33f41ce7 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -33,6 +33,61 @@
>>               'date-sec': 'int', 'date-nsec': 'int',
>>               'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>>   +##
>> +# @BitmapTypeEnum:
>> +#
>> +# An enumeration of possible serialized bitmap types.
>> +#
>> +# @dirty-tracking: This bitmap records information on dirty
>> +#                  segments within the file.
>> +#
>> +# @unknown: This bitmap is of an unknown or reserved type.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>> +
>> +##
>> +# @BitmapFlagEnum:
>> +#
>> +# An enumeration of possible flags for serialized bitmaps.
>> +#
>> +# @in-use: This bitmap is considered to be in-use, and may now be
>> inconsistent.
>> +#
>> +# @auto: This bitmap must reflect any and all changes to the file it
>> describes.
>> +#        The type of this bitmap must be @DirtyTrackingBitmap.
> 
> logical, but I don't see this restriction in the spec. May be we need to
> update the spec
> 
1: auto

"The bitmap must reflect all changes of the virtual disk by any
application that would write to this qcow2 file (including writes,
snapshot switching, etc.). The type of this bitmap must be 'dirty
tracking bitmap'."

Actually, this looks correct now that I'm looking at the spec again.
I've used a terser phrasing but I think it's correct.

>> +#
>> +# @extra-data-compatible: This bitmap has extra information
>> associated with it.
> 
> no, this flag means, that extra data is compatible. So, if you don't
> know what is this extra data, you can read and modify the bitmap,
> leaving this data as is. If this flag is unset, and there are some extra
> data, bitmap must not be used.
> 
> Finally, this spec should be consistent (or, may be better, duplicate)
> spec from docs/interop/qcow2.txt..

I might suggest a rewrite of this portion of the spec as it's a little
unclear to me.

I've given this portion a rewrite.

>> +#
>> +# @unknown: This bitmap has unknown or reserved properties.
> 
> Better is only "reserved flags" (not unknown and not properties), they
> are reserved by spec.
> 
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
>> +                                      'extra-data-compatible',
>> 'unknown' ] }
>> +
>> +##
>> +# @BitmapInfo:
>> +#
>> +# @name: The name of the bitmap.
>> +#
>> +# @type: The type of bitmap.
>> +#
>> +# @granularity: Bitmap granularity, in bytes.
>> +#
>> +# @count: Overall bitmap dirtiness, in bytes.
>> +#
>> +# @flags: Bitmap flags, if any.
>> +#
>> +# Since: 2.13
>> +#
>> +##
>> +{ 'struct': 'BitmapInfo',
>> +  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':
>> 'int',
>> +            'count': 'int', '*flags': ['BitmapFlagEnum']
> 
> may be worth add 'has-extra-data'
> 
>> +  }
>> +}
>> +
>>   ##
>>   # @ImageInfoSpecificQCow2EncryptionBase:
>>   #
>> @@ -69,6 +124,8 @@
>>   # @encrypt: details about encryption parameters; only set if image
>>   #           is encrypted (since 2.10)
>>   #
>> +# @bitmaps: list of image bitmaps (since 2.13)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +134,8 @@
>>         '*lazy-refcounts': 'bool',
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> +      '*bitmaps': ['BitmapInfo']
>>     } }
>>     ##
> 
> 

Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info

[Vladimir Sementsov-Ogievskiy]

17.05.2018 00:15, John Snow wrote:
>
> On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.05.2018 04:25, John Snow wrote:
>>> Add some of the necessary scaffolding for reporting bitmap information.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    qapi/block-core.json | 60
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 59 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index c50517bff3..8f33f41ce7 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -33,6 +33,61 @@
>>>                'date-sec': 'int', 'date-nsec': 'int',
>>>                'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>>>    +##
>>> +# @BitmapTypeEnum:
>>> +#
>>> +# An enumeration of possible serialized bitmap types.
>>> +#
>>> +# @dirty-tracking: This bitmap records information on dirty
>>> +#                  segments within the file.
>>> +#
>>> +# @unknown: This bitmap is of an unknown or reserved type.
>>> +#
>>> +# Since: 2.13
>>> +##
>>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>>> +
>>> +##
>>> +# @BitmapFlagEnum:
>>> +#
>>> +# An enumeration of possible flags for serialized bitmaps.
>>> +#
>>> +# @in-use: This bitmap is considered to be in-use, and may now be
>>> inconsistent.
>>> +#
>>> +# @auto: This bitmap must reflect any and all changes to the file it
>>> describes.
>>> +#        The type of this bitmap must be @DirtyTrackingBitmap.
>> logical, but I don't see this restriction in the spec. May be we need to
>> update the spec
>>
> 1: auto
>
> "The bitmap must reflect all changes of the virtual disk by any
> application that would write to this qcow2 file (including writes,
> snapshot switching, etc.). The type of this bitmap must be 'dirty
> tracking bitmap'."
>
> Actually, this looks correct now that I'm looking at the spec again.
> I've used a terser phrasing but I think it's correct.

another thought: why must? We know nothing about other types.. May be 
for other type this flag will have similar or other meaning.. For me, 
this flag looks like a property of dirty-tracking bitmap, not the thing 
which dictates only that type.

>
>>> +#
>>> +# @extra-data-compatible: This bitmap has extra information
>>> associated with it.
>> no, this flag means, that extra data is compatible. So, if you don't
>> know what is this extra data, you can read and modify the bitmap,
>> leaving this data as is. If this flag is unset, and there are some extra
>> data, bitmap must not be used.
>>
>> Finally, this spec should be consistent (or, may be better, duplicate)
>> spec from docs/interop/qcow2.txt..
> I might suggest a rewrite of this portion of the spec as it's a little
> unclear to me.
>
> I've given this portion a rewrite.
>
>>> +#
>>> +# @unknown: This bitmap has unknown or reserved properties.
>> Better is only "reserved flags" (not unknown and not properties), they
>> are reserved by spec.
>>
>>> +#
>>> +# Since: 2.13
>>> +##
>>> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
>>> +                                      'extra-data-compatible',
>>> 'unknown' ] }
>>> +
>>> +##
>>> +# @BitmapInfo:
>>> +#
>>> +# @name: The name of the bitmap.
>>> +#
>>> +# @type: The type of bitmap.
>>> +#
>>> +# @granularity: Bitmap granularity, in bytes.
>>> +#
>>> +# @count: Overall bitmap dirtiness, in bytes.
>>> +#
>>> +# @flags: Bitmap flags, if any.
>>> +#
>>> +# Since: 2.13
>>> +#
>>> +##
>>> +{ 'struct': 'BitmapInfo',
>>> +  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':
>>> 'int',
>>> +            'count': 'int', '*flags': ['BitmapFlagEnum']
>> may be worth add 'has-extra-data'
>>
>>> +  }
>>> +}
>>> +
>>>    ##
>>>    # @ImageInfoSpecificQCow2EncryptionBase:
>>>    #
>>> @@ -69,6 +124,8 @@
>>>    # @encrypt: details about encryption parameters; only set if image
>>>    #           is encrypted (since 2.10)
>>>    #
>>> +# @bitmaps: list of image bitmaps (since 2.13)
>>> +#
>>>    # Since: 1.7
>>>    ##
>>>    { 'struct': 'ImageInfoSpecificQCow2',
>>> @@ -77,7 +134,8 @@
>>>          '*lazy-refcounts': 'bool',
>>>          '*corrupt': 'bool',
>>>          'refcount-bits': 'int',
>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>> +      '*bitmaps': ['BitmapInfo']
>>>      } }
>>>      ##
>>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info

[John Snow]

On 05/17/2018 06:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> 17.05.2018 00:15, John Snow wrote:
>>
>> On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 12.05.2018 04:25, John Snow wrote:
>>>> Add some of the necessary scaffolding for reporting bitmap information.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    qapi/block-core.json | 60
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 59 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index c50517bff3..8f33f41ce7 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -33,6 +33,61 @@
>>>>                'date-sec': 'int', 'date-nsec': 'int',
>>>>                'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>>>>    +##
>>>> +# @BitmapTypeEnum:
>>>> +#
>>>> +# An enumeration of possible serialized bitmap types.
>>>> +#
>>>> +# @dirty-tracking: This bitmap records information on dirty
>>>> +#                  segments within the file.
>>>> +#
>>>> +# @unknown: This bitmap is of an unknown or reserved type.
>>>> +#
>>>> +# Since: 2.13
>>>> +##
>>>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>>>> +
>>>> +##
>>>> +# @BitmapFlagEnum:
>>>> +#
>>>> +# An enumeration of possible flags for serialized bitmaps.
>>>> +#
>>>> +# @in-use: This bitmap is considered to be in-use, and may now be
>>>> inconsistent.
>>>> +#
>>>> +# @auto: This bitmap must reflect any and all changes to the file it
>>>> describes.
>>>> +#        The type of this bitmap must be @DirtyTrackingBitmap.
>>> logical, but I don't see this restriction in the spec. May be we need to
>>> update the spec
>>>
>> 1: auto
>>
>> "The bitmap must reflect all changes of the virtual disk by any
>> application that would write to this qcow2 file (including writes,
>> snapshot switching, etc.). The type of this bitmap must be 'dirty
>> tracking bitmap'."
>>
>> Actually, this looks correct now that I'm looking at the spec again.
>> I've used a terser phrasing but I think it's correct.
> 
> another thought: why must? We know nothing about other types.. May be
> for other type this flag will have similar or other meaning.. For me,
> this flag looks like a property of dirty-tracking bitmap, not the thing
> which dictates only that type.
> 

Yeah, there may be other kinds that want this behavior. The restriction
can probably be eliminated from the spec, I think, but that's to be
handled outside of this series.

I'll remove the restriction from the QAPI flag, because what "should" or
"must" happen is not relevant to what _is_ happening. :)

--js

[Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info

[John Snow]

Add functions for querying the basic information inside of bitmaps.
Restructure the bitmaps flags masks to facilitate providing a list of
flags belonging to the bitmap(s) being queried.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.c        |  7 +++++
 block/qcow2.h        |  1 +
 3 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 60e01abfd7..811b82743a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -49,8 +49,28 @@
 
 /* Bitmap directory entry flags */
 #define BME_RESERVED_FLAGS 0xfffffffcU
-#define BME_FLAG_IN_USE (1U << 0)
-#define BME_FLAG_AUTO   (1U << 1)
+
+enum BME_FLAG_BITS {
+    BME_FLAG_BIT__BEGIN = 0,
+    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
+    BME_FLAG_BIT_AUTO   = 1,
+    BME_FLAG_BIT_EXTRA  = 2,
+    BME_FLAG_BIT__MAX,
+};
+
+#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
+#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
+#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
+
+/* Correlate canonical spec values to autogenerated QAPI values */
+struct {
+    uint32_t mask;
+    int qapi_value;
+} BMEFlagMap[BME_FLAG_BIT__MAX] = {
+    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE, BITMAP_FLAG_ENUM_IN_USE },
+    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
+    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA,  BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
+};
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
@@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
     }
 }
 
+static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+    int i;
+    BitmapFlagEnumList *flist = NULL;
+    BitmapFlagEnumList *ftmp;
+
+    while (flags) {
+        i = ctz32(flags);
+        ftmp = g_new0(BitmapFlagEnumList, 1);
+        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
+            ftmp->value = BMEFlagMap[i].qapi_value;
+        } else {
+            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
+        }
+        ftmp->next = flist;
+        flist = ftmp;
+        flags &= ~(1 << i);
+    }
+
+    return flist;
+}
+
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs)
+{
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+    BitmapInfoList *info_list = NULL;
+    BitmapInfoList *tmp;
+    BitmapInfo *bitmap_info;
+
+    bm_list = get_bitmap_list(bs, NULL);
+    if (!bm_list) {
+        return info_list;
+    }
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        bitmap_info = g_new0(BitmapInfo, 1);
+        bitmap_info->name = g_strdup(bm->name);
+        if (bm->type == BT_DIRTY_TRACKING_BITMAP) {
+            bitmap_info->type = BITMAP_TYPE_ENUM_DIRTY_TRACKING;
+        } else {
+            bitmap_info->type = BITMAP_TYPE_ENUM_UNKNOWN;
+        }
+        bitmap_info->granularity = 1 << bm->granularity_bits;
+        bitmap_info->count = bdrv_get_dirty_count(bm->dirty_bitmap);
+        bitmap_info->flags = get_bitmap_flags(bm->flags);
+        bitmap_info->has_flags = !!bitmap_info->flags;
+
+        tmp = g_new0(BitmapInfoList, 1);
+        tmp->value = bitmap_info;
+        tmp->next = info_list;
+        info_list = tmp;
+    }
+
+    return info_list;
+}
+
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7ae9000656..0b24ecb6b2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3960,6 +3960,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
     BDRVQcow2State *s = bs->opaque;
     ImageInfoSpecific *spec_info;
     QCryptoBlockInfo *encrypt_info = NULL;
+    BitmapInfoList *bitmap_list = NULL;
 
     if (s->crypto != NULL) {
         encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
@@ -4016,6 +4017,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
         spec_info->u.qcow2.data->encrypt = qencrypt;
     }
 
+    bitmap_list = qcow2_get_bitmap_info(bs);
+    if (bitmap_list) {
+        spec_info->u.qcow2.data->has_bitmaps = true;
+        spec_info->u.qcow2.data->bitmaps = bitmap_list;
+    }
+
     return spec_info;
 }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 796a8c914b..76bf5fa55d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -686,5 +686,6 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
 void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                           const char *name,
                                           Error **errp);
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs);
 
 #endif
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info

[Vladimir Sementsov-Ogievskiy]

12.05.2018 04:25, John Snow wrote:
> Add functions for querying the basic information inside of bitmaps.
> Restructure the bitmaps flags masks to facilitate providing a list of
> flags belonging to the bitmap(s) being queried.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>   block/qcow2.c        |  7 +++++
>   block/qcow2.h        |  1 +
>   3 files changed, 87 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 60e01abfd7..811b82743a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -49,8 +49,28 @@
>   
>   /* Bitmap directory entry flags */
>   #define BME_RESERVED_FLAGS 0xfffffffcU
> -#define BME_FLAG_IN_USE (1U << 0)
> -#define BME_FLAG_AUTO   (1U << 1)
> +
> +enum BME_FLAG_BITS {
> +    BME_FLAG_BIT__BEGIN = 0,
> +    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
> +    BME_FLAG_BIT_AUTO   = 1,
> +    BME_FLAG_BIT_EXTRA  = 2,
> +    BME_FLAG_BIT__MAX,
> +};
> +
> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
> +#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
> +#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
> +
> +/* Correlate canonical spec values to autogenerated QAPI values */
> +struct {
> +    uint32_t mask;

mask is unused in this patch

> +    int qapi_value;
> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
> +    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE, BITMAP_FLAG_ENUM_IN_USE },
> +    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
> +    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA,  BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
> +};
>   
>   /* bits [1, 8] U [56, 63] are reserved */
>   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
>       }
>   }
>   
> +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
> +{
> +    int i;
> +    BitmapFlagEnumList *flist = NULL;
> +    BitmapFlagEnumList *ftmp;
> +
> +    while (flags) {
> +        i = ctz32(flags);
> +        ftmp = g_new0(BitmapFlagEnumList, 1);
> +        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
> +            ftmp->value = BMEFlagMap[i].qapi_value;
> +        } else {
> +            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;

so, there may be several "unknown" entries. It's inconsistent with 
"@unknown: This bitmap has unknown or reserved properties.".

Finally, can we export values for unknown flags? It should be more 
informative.

> +        }
> +        ftmp->next = flist;
> +        flist = ftmp;
> +        flags &= ~(1 << i);
> +    }
> +
> +    return flist;
> +}
> +
> +BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs)
> +{
> +    Qcow2BitmapList *bm_list;
> +    Qcow2Bitmap *bm;
> +    BitmapInfoList *info_list = NULL;
> +    BitmapInfoList *tmp;
> +    BitmapInfo *bitmap_info;
> +
> +    bm_list = get_bitmap_list(bs, NULL);
> +    if (!bm_list) {
> +        return info_list;
> +    }
> +
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        bitmap_info = g_new0(BitmapInfo, 1);
> +        bitmap_info->name = g_strdup(bm->name);
> +        if (bm->type == BT_DIRTY_TRACKING_BITMAP) {
> +            bitmap_info->type = BITMAP_TYPE_ENUM_DIRTY_TRACKING;
> +        } else {
> +            bitmap_info->type = BITMAP_TYPE_ENUM_UNKNOWN;
> +        }
> +        bitmap_info->granularity = 1 << bm->granularity_bits;
> +        bitmap_info->count = bdrv_get_dirty_count(bm->dirty_bitmap);
> +        bitmap_info->flags = get_bitmap_flags(bm->flags);
> +        bitmap_info->has_flags = !!bitmap_info->flags;
> +
> +        tmp = g_new0(BitmapInfoList, 1);
> +        tmp->value = bitmap_info;
> +        tmp->next = info_list;
> +        info_list = tmp;
> +    }
> +
> +    return info_list;
> +}
> +
>   int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>                                     void **refcount_table,
>                                     int64_t *refcount_table_size)
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7ae9000656..0b24ecb6b2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3960,6 +3960,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>       BDRVQcow2State *s = bs->opaque;
>       ImageInfoSpecific *spec_info;
>       QCryptoBlockInfo *encrypt_info = NULL;
> +    BitmapInfoList *bitmap_list = NULL;
>   
>       if (s->crypto != NULL) {
>           encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
> @@ -4016,6 +4017,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>           spec_info->u.qcow2.data->encrypt = qencrypt;
>       }
>   
> +    bitmap_list = qcow2_get_bitmap_info(bs);
> +    if (bitmap_list) {
> +        spec_info->u.qcow2.data->has_bitmaps = true;
> +        spec_info->u.qcow2.data->bitmaps = bitmap_list;
> +    }
> +
>       return spec_info;
>   }
>   
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 796a8c914b..76bf5fa55d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -686,5 +686,6 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>   void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>                                             const char *name,
>                                             Error **errp);
> +BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs);
>   
>   #endif


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info

[John Snow]

On 05/14/2018 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add functions for querying the basic information inside of bitmaps.
>> Restructure the bitmaps flags masks to facilitate providing a list of
>> flags belonging to the bitmap(s) being queried.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 81
>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   block/qcow2.c        |  7 +++++
>>   block/qcow2.h        |  1 +
>>   3 files changed, 87 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 60e01abfd7..811b82743a 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -49,8 +49,28 @@
>>     /* Bitmap directory entry flags */
>>   #define BME_RESERVED_FLAGS 0xfffffffcU
>> -#define BME_FLAG_IN_USE (1U << 0)
>> -#define BME_FLAG_AUTO   (1U << 1)
>> +
>> +enum BME_FLAG_BITS {
>> +    BME_FLAG_BIT__BEGIN = 0,
>> +    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
>> +    BME_FLAG_BIT_AUTO   = 1,
>> +    BME_FLAG_BIT_EXTRA  = 2,
>> +    BME_FLAG_BIT__MAX,
>> +};
>> +
>> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
>> +#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
>> +#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
>> +
>> +/* Correlate canonical spec values to autogenerated QAPI values */
>> +struct {
>> +    uint32_t mask;
> 
> mask is unused in this patch
> 

It sure is! I'll remove it. It's an artifact from an earlier revision.

>> +    int qapi_value;
>> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
>> +    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
>> BITMAP_FLAG_ENUM_IN_USE },
>> +    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
>> +    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA, 
>> BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
>> +};
>>     /* bits [1, 8] U [56, 63] are reserved */
>>   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
>> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
>>       }
>>   }
>>   +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
>> +{
>> +    int i;
>> +    BitmapFlagEnumList *flist = NULL;
>> +    BitmapFlagEnumList *ftmp;
>> +
>> +    while (flags) {
>> +        i = ctz32(flags);
>> +        ftmp = g_new0(BitmapFlagEnumList, 1);
>> +        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
>> +            ftmp->value = BMEFlagMap[i].qapi_value;
>> +        } else {
>> +            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
> 
> so, there may be several "unknown" entries. It's inconsistent with
> "@unknown: This bitmap has unknown or reserved properties.".
> 

Based on your feedback from the previous patch I'll probably just call
this @reserved for any unrecognized flag.

> Finally, can we export values for unknown flags? It should be more
> informative.
> 

I'll see what I can do -- I hate losing this information too.

Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info

[John Snow]

On 05/14/2018 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add functions for querying the basic information inside of bitmaps.
>> Restructure the bitmaps flags masks to facilitate providing a list of
>> flags belonging to the bitmap(s) being queried.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 81
>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   block/qcow2.c        |  7 +++++
>>   block/qcow2.h        |  1 +
>>   3 files changed, 87 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 60e01abfd7..811b82743a 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -49,8 +49,28 @@
>>     /* Bitmap directory entry flags */
>>   #define BME_RESERVED_FLAGS 0xfffffffcU
>> -#define BME_FLAG_IN_USE (1U << 0)
>> -#define BME_FLAG_AUTO   (1U << 1)
>> +
>> +enum BME_FLAG_BITS {
>> +    BME_FLAG_BIT__BEGIN = 0,
>> +    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
>> +    BME_FLAG_BIT_AUTO   = 1,
>> +    BME_FLAG_BIT_EXTRA  = 2,
>> +    BME_FLAG_BIT__MAX,
>> +};
>> +
>> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
>> +#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
>> +#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
>> +
>> +/* Correlate canonical spec values to autogenerated QAPI values */
>> +struct {
>> +    uint32_t mask;
> 
> mask is unused in this patch
> 
>> +    int qapi_value;
>> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
>> +    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
>> BITMAP_FLAG_ENUM_IN_USE },
>> +    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
>> +    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA, 
>> BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
>> +};
>>     /* bits [1, 8] U [56, 63] are reserved */
>>   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
>> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
>>       }
>>   }
>>   +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
>> +{
>> +    int i;
>> +    BitmapFlagEnumList *flist = NULL;
>> +    BitmapFlagEnumList *ftmp;
>> +
>> +    while (flags) {
>> +        i = ctz32(flags);
>> +        ftmp = g_new0(BitmapFlagEnumList, 1);
>> +        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
>> +            ftmp->value = BMEFlagMap[i].qapi_value;
>> +        } else {
>> +            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
> 
> so, there may be several "unknown" entries. It's inconsistent with
> "@unknown: This bitmap has unknown or reserved properties.".
> 
> Finally, can we export values for unknown flags? It should be more
> informative.
> 

I changed my mind -- qemu-img is not a debugging tool and shouldn't get
lost in the weeds trying to enumerate the different kinds of reserved
values that are set.

It's an error to have them set, or not -- which ones specifically is not
information that an end-user need know or care about, and I don't want
to create an extra structure to contain the value.

I'm going to rewrite this loop to just only ever have one unknown value
at a maximum.

--js

Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info

[Vladimir Sementsov-Ogievskiy]

17.05.2018 00:17, John Snow wrote:
>
> On 05/14/2018 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.05.2018 04:25, John Snow wrote:
>>> Add functions for querying the basic information inside of bitmaps.
>>> Restructure the bitmaps flags masks to facilitate providing a list of
>>> flags belonging to the bitmap(s) being queried.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    block/qcow2-bitmap.c | 81
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>    block/qcow2.c        |  7 +++++
>>>    block/qcow2.h        |  1 +
>>>    3 files changed, 87 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 60e01abfd7..811b82743a 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -49,8 +49,28 @@
>>>      /* Bitmap directory entry flags */
>>>    #define BME_RESERVED_FLAGS 0xfffffffcU
>>> -#define BME_FLAG_IN_USE (1U << 0)
>>> -#define BME_FLAG_AUTO   (1U << 1)
>>> +
>>> +enum BME_FLAG_BITS {
>>> +    BME_FLAG_BIT__BEGIN = 0,
>>> +    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
>>> +    BME_FLAG_BIT_AUTO   = 1,
>>> +    BME_FLAG_BIT_EXTRA  = 2,
>>> +    BME_FLAG_BIT__MAX,
>>> +};
>>> +
>>> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
>>> +#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
>>> +#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
>>> +
>>> +/* Correlate canonical spec values to autogenerated QAPI values */
>>> +struct {
>>> +    uint32_t mask;
>> mask is unused in this patch
>>
>>> +    int qapi_value;
>>> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
>>> +    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
>>> BITMAP_FLAG_ENUM_IN_USE },
>>> +    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
>>> +    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA,
>>> BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
>>> +};
>>>      /* bits [1, 8] U [56, 63] are reserved */
>>>    #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
>>> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
>>>        }
>>>    }
>>>    +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
>>> +{
>>> +    int i;
>>> +    BitmapFlagEnumList *flist = NULL;
>>> +    BitmapFlagEnumList *ftmp;
>>> +
>>> +    while (flags) {
>>> +        i = ctz32(flags);
>>> +        ftmp = g_new0(BitmapFlagEnumList, 1);
>>> +        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
>>> +            ftmp->value = BMEFlagMap[i].qapi_value;
>>> +        } else {
>>> +            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
>> so, there may be several "unknown" entries. It's inconsistent with
>> "@unknown: This bitmap has unknown or reserved properties.".
>>
>> Finally, can we export values for unknown flags? It should be more
>> informative.
>>
> I changed my mind -- qemu-img is not a debugging tool and shouldn't get
> lost in the weeds trying to enumerate the different kinds of reserved
> values that are set.
>
> It's an error to have them set, or not -- which ones specifically is not
> information that an end-user need know or care about, and I don't want
> to create an extra structure to contain the value.
>
> I'm going to rewrite this loop to just only ever have one unknown value
> at a maximum.
>
> --js
>

Ok, I agree.

-- 
Best regards,
Vladimir

[Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent

[John Snow]

The function already exists, just expose it.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 include/qapi/qmp/qjson.h |  1 +
 qobject/qjson.c          | 21 +++++++++++----------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index b274ac3a86..0e8624c1fa 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
 QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
     GCC_FMT_ATTR(1, 0);
 
+QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent);
 QString *qobject_to_json(const QObject *obj);
 QString *qobject_to_json_pretty(const QObject *obj);
 
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 9816a65c7d..d603e1cce1 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -252,20 +252,21 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     }
 }
 
+QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent)
+{
+    QString *str = qstring_new();
+
+    to_json(obj, str, pretty, indent);
+
+    return str;
+}
+
 QString *qobject_to_json(const QObject *obj)
 {
-    QString *str = qstring_new();
-
-    to_json(obj, str, 0, 0);
-
-    return str;
+    return qobject_to_json_opt(obj, 0, 0);
 }
 
 QString *qobject_to_json_pretty(const QObject *obj)
 {
-    QString *str = qstring_new();
-
-    to_json(obj, str, 1, 0);
-
-    return str;
+    return qobject_to_json_opt(obj, 1, 0);
 }
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent

[Eric Blake]

On 05/11/2018 08:25 PM, John Snow wrote:
> The function already exists, just expose it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   include/qapi/qmp/qjson.h |  1 +
>   qobject/qjson.c          | 21 +++++++++++----------
>   2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
> index b274ac3a86..0e8624c1fa 100644
> --- a/include/qapi/qmp/qjson.h
> +++ b/include/qapi/qmp/qjson.h
> @@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
>   QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>       GCC_FMT_ATTR(1, 0);
>   
> +QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent);

pretty seems like it is better as a bool.  Even though to_json() isn't 
typed correctly, that's no excuse for the public function to copy a bad 
interface (and maybe it's worth a separate cleanup patch to fix 
to_json() first).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent

[John Snow]

On 05/16/2018 05:34 PM, Eric Blake wrote:
> On 05/11/2018 08:25 PM, John Snow wrote:
>> The function already exists, just expose it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   include/qapi/qmp/qjson.h |  1 +
>>   qobject/qjson.c          | 21 +++++++++++----------
>>   2 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
>> index b274ac3a86..0e8624c1fa 100644
>> --- a/include/qapi/qmp/qjson.h
>> +++ b/include/qapi/qmp/qjson.h
>> @@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>> GCC_FMT_ATTR(1, 2);
>>   QObject *qobject_from_jsonv(const char *string, va_list *ap, Error
>> **errp)
>>       GCC_FMT_ATTR(1, 0);
>>   +QString *qobject_to_json_opt(const QObject *obj, int pretty, int
>> indent);
> 
> pretty seems like it is better as a bool.  Even though to_json() isn't
> typed correctly, that's no excuse for the public function to copy a bad
> interface (and maybe it's worth a separate cleanup patch to fix
> to_json() first).
> 

Eh, sure, why not. Done.

[Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs

[John Snow]

Add two new structures for detailing the marked regions of bitmaps as
saved in e.g. qcow2 files.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f33f41ce7..de8ad73a78 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -298,6 +298,38 @@
            'zero': 'bool', 'depth': 'int', '*offset': 'int',
            '*filename': 'str' } }
 
+##
+# @BitmapEntry:
+#
+# Dirty Bitmap region information for a virtual block range
+#
+# @offset: the start byte of the dirty virtual range
+#
+# @length: the number of bytes of the dirty virtual range
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapEntry',
+  'data': { 'offset': 'int', 'length': 'int' } }
+
+##
+# @BitmapMapping:
+#
+# List of described regions correlated to a named bitmap.
+#
+# @name: The name of the bitmap whose range is described here
+#
+# @entries: A list of zero or more @BitmapEntry elements representing
+#           the range(s) described by the bitmap.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapMapping',
+  'data': { 'name': 'str',
+            'entries': [ 'BitmapEntry' ] } }
+
 ##
 # @BlockdevCacheInfo:
 #
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs

[Eric Blake]

On 05/11/2018 08:25 PM, John Snow wrote:
> Add two new structures for detailing the marked regions of bitmaps as
> saved in e.g. qcow2 files.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   qapi/block-core.json | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8f33f41ce7..de8ad73a78 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -298,6 +298,38 @@
>              'zero': 'bool', 'depth': 'int', '*offset': 'int',
>              '*filename': 'str' } }
>   
> +##
> +# @BitmapEntry:
> +#
> +# Dirty Bitmap region information for a virtual block range
> +#
> +# @offset: the start byte of the dirty virtual range
> +#
> +# @length: the number of bytes of the dirty virtual range
> +#
> +# Since: 2.13
> +#
> +##
> +{ 'struct': 'BitmapEntry',
> +  'data': { 'offset': 'int', 'length': 'int' } }
> +
> +##
> +# @BitmapMapping:
> +#
> +# List of described regions correlated to a named bitmap.
> +#
> +# @name: The name of the bitmap whose range is described here
> +#
> +# @entries: A list of zero or more @BitmapEntry elements representing
> +#           the range(s) described by the bitmap.

Is it also worth documenting that the list will be in ascending order, 
with no overlaps (no two entries covering the same offset); and in fact 
with a gap between all entries (as otherwise those two consecutive 
entries could have been consolidated to one)?

> +#
> +# Since: 2.13
> +#
> +##
> +{ 'struct': 'BitmapMapping',
> +  'data': { 'name': 'str',
> +            'entries': [ 'BitmapEntry' ] } }
> +
>   ##
>   # @BlockdevCacheInfo:
>   #
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs

[John Snow]

On 05/16/2018 05:37 PM, Eric Blake wrote:
>> +##
>> +# @BitmapMapping:
>> +#
>> +# List of described regions correlated to a named bitmap.
>> +#
>> +# @name: The name of the bitmap whose range is described here
>> +#
>> +# @entries: A list of zero or more @BitmapEntry elements representing
>> +#           the range(s) described by the bitmap.
> 
> Is it also worth documenting that the list will be in ascending order,
> with no overlaps (no two entries covering the same offset); and in fact
> with a gap between all entries (as otherwise those two consecutive
> entries could have been consolidated to one)?
> 

Hm. I didn't necessarily want to guarantee the order, but being more
specific about the output will assist legibility of the spec.

I'll amend the documentation and make some stronger guarantees.

>> +#
>> +# Since: 2.13
>> +#
>> +##
>> +{ 'struct': 'BitmapMapping',
>> +  'data': { 'name': 'str',
>> +            'entries': [ 'BitmapEntry' ] } }
>> +
>>   ##
>>   # @BlockdevCacheInfo:
>>   # 

[Qemu-devel] [RFC PATCH 10/12] qemu-img: split off common chunk of map command

[John Snow]

It will be re-used for a bitmap listing command.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-img.c | 192 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 110 insertions(+), 82 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ea62d2d61e..e31e38f674 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -69,8 +69,8 @@ enum {
 };
 
 typedef enum OutputFormat {
-    OFORMAT_JSON,
     OFORMAT_HUMAN,
+    OFORMAT_JSON,
 } OutputFormat;
 
 /* Default to cache=writeback as data integrity is not important for qemu-img */
@@ -2728,96 +2728,122 @@ static inline bool entry_mergeable(const MapEntry *curr, const MapEntry *next)
     return true;
 }
 
+typedef struct CommonOpts {
+    OutputFormat output_format;
+    const char *filename;
+    const char *fmt;
+    bool force_share;
+    bool image_opts;
+} CommonOpts;
+
+static int parse_opts_common(CommonOpts *opts, int argc, char **argv)
+{
+    int c;
+
+    for (;;) {
+        int option_index = 0;
+        static const struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"format", required_argument, 0, 'f'},
+            {"output", required_argument, 0, OPTION_OUTPUT},
+            {"object", required_argument, 0, OPTION_OBJECT},
+            {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"force-share", no_argument, 0, 'U'},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, ":f:hU",
+                        long_options, &option_index);
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case ':':
+            missing_argument(argv[optind - 1]);
+            break;
+        case '?':
+            unrecognized_option(argv[optind - 1]);
+            break;
+        case 'h':
+            help();
+            break;
+        case 'f':
+            opts->fmt = optarg;
+            break;
+        case 'U':
+            opts->force_share = true;
+            break;
+        case OPTION_OUTPUT:
+            if (optarg && !strcmp(optarg, "json")) {
+                opts->output_format = OFORMAT_JSON;
+            } else if (optarg && !strcmp(optarg, "human")) {
+                opts->output_format = OFORMAT_HUMAN;
+            } else {
+                error_report("--output must be used with human or json as argument.");
+                return -EINVAL;
+            }
+            break;
+        case OPTION_OBJECT: {
+            QemuOpts *opts;
+            opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                           optarg, true);
+            if (!opts) {
+                return -EINVAL;
+            }
+        }   break;
+        case OPTION_IMAGE_OPTS:
+            opts->image_opts = true;
+            break;
+        }
+    }
+    if (qemu_opts_foreach(&qemu_object_opts,
+                          user_creatable_add_opts_foreach,
+                          NULL, NULL)) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static void parse_positional(int argc, char *argv[],
+                             bool opt, const char **str, const char *name) {
+    if (!opt && optind >= argc) {
+        error_exit("Expecting '%s' positional parameter", name);
+    } else if (optind < argc) {
+        *str = argv[optind++];
+    }
+}
+
+static void parse_unexpected(int argc, char *argv[]) {
+    if (optind != argc) {
+        error_exit("Unexpected argument '%s'\n", argv[optind]);
+    }
+}
+
 static int img_map(int argc, char **argv)
 {
-    int c;
-    OutputFormat output_format = OFORMAT_HUMAN;
     BlockBackend *blk;
     BlockDriverState *bs;
-    const char *filename, *fmt, *output;
     int64_t length;
+    int ret = 0;
     MapEntry curr = { .length = 0 }, next;
-    int ret = 0;
-    bool image_opts = false;
-    bool force_share = false;
+    CommonOpts *opts;
 
-    fmt = NULL;
-    output = NULL;
-    for (;;) {
-        int option_index = 0;
-        static const struct option long_options[] = {
-            {"help", no_argument, 0, 'h'},
-            {"format", required_argument, 0, 'f'},
-            {"output", required_argument, 0, OPTION_OUTPUT},
-            {"object", required_argument, 0, OPTION_OBJECT},
-            {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
-            {"force-share", no_argument, 0, 'U'},
-            {0, 0, 0, 0}
-        };
-        c = getopt_long(argc, argv, ":f:hU",
-                        long_options, &option_index);
-        if (c == -1) {
-            break;
-        }
-        switch (c) {
-        case ':':
-            missing_argument(argv[optind - 1]);
-            break;
-        case '?':
-            unrecognized_option(argv[optind - 1]);
-            break;
-        case 'h':
-            help();
-            break;
-        case 'f':
-            fmt = optarg;
-            break;
-        case 'U':
-            force_share = true;
-            break;
-        case OPTION_OUTPUT:
-            output = optarg;
-            break;
-        case OPTION_OBJECT: {
-            QemuOpts *opts;
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                return 1;
-            }
-        }   break;
-        case OPTION_IMAGE_OPTS:
-            image_opts = true;
-            break;
-        }
-    }
-    if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
-    }
-    filename = argv[optind];
-
-    if (output && !strcmp(output, "json")) {
-        output_format = OFORMAT_JSON;
-    } else if (output && !strcmp(output, "human")) {
-        output_format = OFORMAT_HUMAN;
-    } else if (output) {
-        error_report("--output must be used with human or json as argument.");
-        return 1;
-    }
-
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          NULL, NULL)) {
-        return 1;
+    opts = g_new0(CommonOpts, 1);
+    if (parse_opts_common(opts, argc, argv)) {
+        return EXIT_FAILURE;
     }
+    parse_positional(argc, argv, 0, &opts->filename, "filename");
+    parse_unexpected(argc, argv);
 
-    blk = img_open(image_opts, filename, fmt, 0, false, false, force_share);
+    blk = img_open(opts->image_opts, opts->filename, opts->fmt, 0,
+                   false, false, opts->force_share);
     if (!blk) {
-        return 1;
+        ret = -1;
+        goto out1;
     }
     bs = blk_bs(blk);
 
-    if (output_format == OFORMAT_HUMAN) {
+    if (opts->output_format == OFORMAT_HUMAN) {
         printf("%-16s%-16s%-16s%s\n", "Offset", "Length", "Mapped to", "File");
     }
 
@@ -2832,7 +2858,7 @@ static int img_map(int argc, char **argv)
 
         if (ret < 0) {
             error_report("Could not read file metadata: %s", strerror(-ret));
-            goto out;
+            goto out2;
         }
 
         if (entry_mergeable(&curr, &next)) {
@@ -2841,15 +2867,17 @@ static int img_map(int argc, char **argv)
         }
 
         if (curr.length > 0) {
-            dump_map_entry(output_format, &curr, &next);
+            dump_map_entry(opts->output_format, &curr, &next);
         }
         curr = next;
     }
 
-    dump_map_entry(output_format, &curr, NULL);
+    dump_map_entry(opts->output_format, &curr, NULL);
 
-out:
+out2:
     blk_unref(blk);
+out1:
+    g_free(opts);
     return ret < 0;
 }
 
-- 
2.14.3

[Qemu-devel] [RFC PATCH 11/12] qemu-img: add bitmap dump

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-img-cmds.hx |   6 ++
 qemu-img.c       | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 191 insertions(+)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 2fe31893cf..d25f359f5a 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,6 +22,12 @@ STEXI
 @item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] [--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] [--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t @var{cache}] [-w] [-U] @var{filename}
 ETEXI
 
+DEF("bitmap", img_bitmap,
+    "bitmap dump [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename [bitmap]")
+STEXI
+@item bitmap dump [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
+ETEXI
+
 DEF("check", img_check,
     "check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
 STEXI
diff --git a/qemu-img.c b/qemu-img.c
index e31e38f674..1141216617 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2881,6 +2881,191 @@ out1:
     return ret < 0;
 }
 
+static BitmapMapping *get_bitmap_mapping(BdrvDirtyBitmap *bitmap)
+{
+    BdrvDirtyBitmapIter *it = bdrv_dirty_iter_new(bitmap);
+    BitmapMapping *bm = g_new0(BitmapMapping, 1);
+    BitmapEntry *entry;
+    BitmapEntryList *tmp, **last;
+    int64_t offset;
+    int64_t zero_offset;
+
+    bm->name = g_strdup(bdrv_dirty_bitmap_name(bitmap));
+    bm->entries = NULL;
+    last = &bm->entries;
+
+    while (true) {
+        offset = bdrv_dirty_iter_next(it);
+        if (offset == -1) {
+            break;
+        }
+
+        zero_offset = bdrv_dirty_bitmap_next_zero(bitmap, offset);
+        if (zero_offset == -1) {
+            zero_offset = bdrv_dirty_bitmap_size(bitmap);
+        }
+
+        entry = g_new0(BitmapEntry, 1);
+        entry->offset = offset;
+        entry->length = zero_offset - offset;
+
+        tmp = g_new0(BitmapEntryList, 1);
+        tmp->value = entry;
+        *last = tmp;
+        last = &tmp->next;
+
+        if (zero_offset >= bdrv_dirty_bitmap_size(bitmap)) {
+            break;
+        }
+        bdrv_set_dirty_iter(it, zero_offset);
+    }
+
+    bdrv_dirty_iter_free(it);
+    return bm;
+}
+
+static void dump_bitmap_mapping_json(BitmapMapping *bm, int indent,
+                                     FILE *stream)
+{
+    QString *str;
+    QObject *obj;
+    Visitor *v = qobject_output_visitor_new(&obj);
+    const int pretty = 1;
+
+    visit_type_BitmapMapping(v, NULL, &bm, &error_abort);
+    visit_complete(v, &obj);
+    str = qobject_to_json_opt(obj, pretty, indent);
+    assert(str != NULL);
+    fprintf(stream, "%*s%s", 4 * indent, "", qstring_get_str(str));
+    qobject_unref(obj);
+    visit_free(v);
+    qobject_unref(str);
+}
+
+static void dump_bitmap_mapping_human(BitmapMapping *bm, FILE *stream)
+{
+    BitmapEntryList *bl = bm->entries;
+
+    fprintf(stream, "# %s\n", bm->name);
+    fprintf(stream, "%-16s%-16s\n", "Offset", "Length");
+    for ( ; bl; bl = bl->next) {
+        BitmapEntry *be = bl->value;
+        fprintf(stream, "%-16"PRId64"%-16"PRId64"\n", be->offset, be->length);
+    }
+}
+
+static void dump_bitmap_mapping(BitmapMapping *bm, OutputFormat output_format,
+                                int indent, FILE *stream)
+{
+    switch (output_format) {
+    case OFORMAT_JSON:
+        dump_bitmap_mapping_json(bm, indent, stream);
+        return;
+    case OFORMAT_HUMAN:
+        dump_bitmap_mapping_human(bm, stream);
+        return;
+    }
+}
+
+/* img_bitmap subcommands: dump */
+
+typedef struct BitmapOpts {
+    CommonOpts base;
+    const char *name;
+    uint32_t granularity;
+} BitmapOpts;
+
+static int bitmap_cmd_dump(BlockDriverState *bs, BitmapOpts *opts)
+{
+    BdrvDirtyBitmap *bitmap;
+    BitmapMapping *bm;
+    bool printed;
+    OutputFormat ofmt = opts->base.output_format;
+
+    if (opts->name) {
+        bitmap = bdrv_find_dirty_bitmap(bs, opts->name);
+        if (bitmap) {
+            bm = get_bitmap_mapping(bitmap);
+            dump_bitmap_mapping(bm, ofmt, 0, stdout);
+            qapi_free_BitmapMapping(bm);
+        } else {
+            if (ofmt == OFORMAT_HUMAN) {
+                fprintf(stdout, "Bitmap '%s' not found\n", opts->name);
+            }
+        }
+    } else {
+        if (ofmt == OFORMAT_JSON) {
+            fprintf(stdout, "[\n");
+        }
+        for (bitmap = NULL, printed = false;
+             (bitmap = bdrv_dirty_bitmap_next(bs, bitmap)); printed = true) {
+            if (printed) {
+                fprintf(stdout, ofmt == OFORMAT_JSON ? ",\n" : "\n");
+            }
+            bm = get_bitmap_mapping(bitmap);
+            dump_bitmap_mapping(bm, ofmt, 1, stdout);
+            qapi_free_BitmapMapping(bm);
+        }
+        if (ofmt == OFORMAT_JSON) {
+            fprintf(stdout, "%s%s", printed ? "\n" : "", "]\n");
+        }
+        if (ofmt == OFORMAT_HUMAN && printed == false) {
+            fprintf(stdout, "No bitmaps are present in this image.\n");
+        }
+    }
+
+    return 0;
+}
+
+static int img_bitmap(int argc, char **argv)
+{
+    const char *cmd;
+    BitmapOpts *opts;
+    BlockBackend *blk;
+    BlockDriverState *bs;
+    const char *bname = NULL;
+    int flags = 0;
+    int ret = EXIT_SUCCESS;
+    int (* handler)(BlockDriverState *b, BitmapOpts *opts);
+
+    if (argc < 2) {
+        error_report("Expected a bitmap subcommand: <dump>");
+        return EXIT_FAILURE;
+    }
+    cmd = argv[1];
+    if (strcmp(cmd, "dump") == 0) {
+        handler = bitmap_cmd_dump;
+    } else {
+        error_report("Unrecognized bitmap subcommand '%s'", cmd);
+        return EXIT_FAILURE;
+    }
+
+    opts = g_new0(BitmapOpts, 1);
+    if (parse_opts_common(&opts->base, --argc, ++argv)) {
+        return EXIT_FAILURE;
+    }
+    parse_positional(argc, argv, 0, &opts->base.filename, "filename");
+    parse_positional(argc, argv, 1, &bname, "bitmap");
+    parse_unexpected(argc, argv);
+
+    blk = img_open(opts->base.image_opts, opts->base.filename, opts->base.fmt,
+                   flags, false, false, opts->base.force_share);
+    if (!blk) {
+        ret = EXIT_FAILURE;
+        goto out;
+    }
+    bs = blk_bs(blk);
+
+    if (handler(bs, opts)) {
+        ret = EXIT_FAILURE;
+    }
+
+    blk_unref(blk);
+ out:
+    g_free(opts);
+    return ret;
+}
+
 #define SNAPSHOT_LIST   1
 #define SNAPSHOT_CREATE 2
 #define SNAPSHOT_APPLY  3
-- 
2.14.3

[Qemu-devel] [RFC PATCH 12/12] qemu-img: add bitmap clear

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-img-cmds.hx |  4 ++--
 qemu-img.c       | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index d25f359f5a..7b6ec73488 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -23,9 +23,9 @@ STEXI
 ETEXI
 
 DEF("bitmap", img_bitmap,
-    "bitmap dump [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename [bitmap]")
+    "bitmap <dump|clear> [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename [bitmap]")
 STEXI
-@item bitmap dump [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
+@item bitmap <dump|clear> [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
 ETEXI
 
 DEF("check", img_check,
diff --git a/qemu-img.c b/qemu-img.c
index 1141216617..b5ab8a3837 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2967,7 +2967,22 @@ static void dump_bitmap_mapping(BitmapMapping *bm, OutputFormat output_format,
     }
 }
 
-/* img_bitmap subcommands: dump */
+static int do_bitmap_clear(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    bool enabled = bdrv_dirty_bitmap_enabled(bitmap);
+
+    if (!enabled) {
+        bdrv_enable_dirty_bitmap(bitmap);
+    }
+    bdrv_clear_dirty_bitmap(bitmap, NULL);
+    if (!enabled) {
+        bdrv_disable_dirty_bitmap(bitmap);
+    }
+
+    return 0;
+}
+
+/* img_bitmap subcommands: dump, clear */
 
 typedef struct BitmapOpts {
     CommonOpts base;
@@ -3017,6 +3032,30 @@ static int bitmap_cmd_dump(BlockDriverState *bs, BitmapOpts *opts)
     return 0;
 }
 
+static int bitmap_cmd_clear(BlockDriverState *bs, BitmapOpts *opts)
+{
+    BdrvDirtyBitmap *bitmap = NULL;
+
+    if (opts->name) {
+        bitmap = bdrv_find_dirty_bitmap(bs, opts->name);
+        if (!bitmap) {
+            error_report("No bitmap named '%s' found", opts->name);
+            return -1;
+        }
+        if (do_bitmap_clear(bs, bitmap)) {
+            return -1;
+        }
+    } else {
+        while ((bitmap = bdrv_dirty_bitmap_next(bs, bitmap))) {
+            if (do_bitmap_clear(bs, bitmap)) {
+                return -1;
+            }
+        }
+    }
+
+    return 0;
+}
+
 static int img_bitmap(int argc, char **argv)
 {
     const char *cmd;
@@ -3029,12 +3068,15 @@ static int img_bitmap(int argc, char **argv)
     int (* handler)(BlockDriverState *b, BitmapOpts *opts);
 
     if (argc < 2) {
-        error_report("Expected a bitmap subcommand: <dump>");
+        error_report("Expected a bitmap subcommand: <dump | clear>");
         return EXIT_FAILURE;
     }
     cmd = argv[1];
     if (strcmp(cmd, "dump") == 0) {
         handler = bitmap_cmd_dump;
+    } else if (strcmp(cmd, "clear") == 0) {
+        flags |= BDRV_O_RDWR;
+        handler = bitmap_cmd_clear;
     } else {
         error_report("Unrecognized bitmap subcommand '%s'", cmd);
         return EXIT_FAILURE;
-- 
2.14.3

Re: [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180512012537.22478-1-jsnow@redhat.com
Subject: [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20180512012537.22478-1-jsnow@redhat.com -> patchew/20180512012537.22478-1-jsnow@redhat.com
Switched to a new branch 'test'
98c78cde32 qemu-img: add bitmap clear
64b46d376f qemu-img: add bitmap dump
e820e6d20d qemu-img: split off common chunk of map command
15894bec9b qapi/block-core: add BitmapMapping and BitmapEntry structs
a09d1c3a65 qjson: allow caller to ask for arbitrary indent
384e244766 qcow2-bitmap: add basic bitmaps info
35cf9ace22 qapi: add bitmap info
36be89af25 qcow2-bitmap: track bitmap type
d347564ea3 qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
c554bff42b block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
9d00c36fba qcow2/dirty-bitmap: cache loaded bitmaps
7f34c78783 qcow2-bitmap: cache bm_list

=== OUTPUT BEGIN ===
Checking PATCH 1/12: qcow2-bitmap: cache bm_list...
Checking PATCH 2/12: qcow2/dirty-bitmap: cache loaded bitmaps...
Checking PATCH 3/12: block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps...
Checking PATCH 4/12: qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO...
Checking PATCH 5/12: qcow2-bitmap: track bitmap type...
Checking PATCH 6/12: qapi: add bitmap info...
Checking PATCH 7/12: qcow2-bitmap: add basic bitmaps info...
WARNING: line over 80 characters
#43: FILE: block/qcow2-bitmap.c:72:
+    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA,  BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },

total: 0 errors, 1 warnings, 118 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 8/12: qjson: allow caller to ask for arbitrary indent...
Checking PATCH 9/12: qapi/block-core: add BitmapMapping and BitmapEntry structs...
Checking PATCH 10/12: qemu-img: split off common chunk of map command...
WARNING: line over 80 characters
#74: FILE: qemu-img.c:2781:
+                error_report("--output must be used with human or json as argument.");

ERROR: open brace '{' following function declarations go on the next line
#127: FILE: qemu-img.c:2816:
+static void parse_unexpected(int argc, char *argv[]) {

total: 1 errors, 1 warnings, 173 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 11/12: qemu-img: add bitmap dump...
ERROR: space prohibited after that '*' (ctx:BxW)
#179: FILE: qemu-img.c:3029:
+    int (* handler)(BlockDriverState *b, BitmapOpts *opts);
          ^

total: 1 errors, 0 warnings, 203 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 12/12: qemu-img: add bitmap clear...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries

[Vladimir Sementsov-Ogievskiy]

12.05.2018 04:25, John Snow wrote:
> Allow qemu-img to show information about bitmaps stored in qcow2 images.
> Add a 'bitmap' meta-command with 'dump' sub-command to retrieve a list of
> dirty regions in bitmaps stored in a qcow2 image.
>
> RFC:
> - I am not 1000% convinced the bm_list caching is perfectly safe,
>    especially with respect to migration and inactivation. There are
>    more efficiencies and tweaks I can make, but I think this is the
>    minimal set.
>
> - I decided not to gate bitmap info in the "info" command behind
>    extra flags.
>
> - Bitmap data-gathering in "bitmap dump" could be made more
>    space-efficient by just reporting one segment at a time,
>    probably.
>
> - `bitmap rm` and `bitmap mk` need extra work, so I am not submitting
>    them just yet: rm needs more work around the remove persistence API,
>    and make needs more work around the "can add" API.
>
> - None of these commands will work with "in use" bitmaps; we need
>    qemu-img check -r bitmaps support for this. I'm not sure what the
>    right behavior to "fix" in-use bitmaps should be:
>          - Cleared: This might be dangerous.
>          - Fully Set: This is safer, but stupid.
>          - Deleted: This might be the best option.

I think it's the only valid option.

>          - Un-set in-use: VERY dangerous; would rather not.
>
> John Snow (12):
>    qcow2-bitmap: cache bm_list
>    qcow2/dirty-bitmap: cache loaded bitmaps
>    block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
>    qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
>    qcow2-bitmap: track bitmap type
>    qapi: add bitmap info
>    qcow2-bitmap: add basic bitmaps info
>    qjson: allow caller to ask for arbitrary indent
>    qapi/block-core: add BitmapMapping and BitmapEntry structs
>    qemu-img: split off common chunk of map command
>    qemu-img: add bitmap dump
>    qemu-img: add bitmap clear
>
>   block/qcow2-bitmap.c     | 220 +++++++++++++++++--------
>   block/qcow2.c            |   9 +
>   block/qcow2.h            |   3 +
>   include/qapi/qmp/qjson.h |   1 +
>   qapi/block-core.json     |  92 ++++++++++-
>   qemu-img-cmds.hx         |   6 +
>   qemu-img.c               | 419 +++++++++++++++++++++++++++++++++++++----------
>   qobject/qjson.c          |  21 +--
>   8 files changed, 612 insertions(+), 159 deletions(-)
>


-- 
Best regards,
Vladimir