[18/22] qapi: add md5 checksum of last dirty bitmap level to query-block
diff mbox

Message ID 1475232808-4852-19-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 30, 2016, 10:53 a.m. UTC
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c   | 1 +
 include/qemu/hbitmap.h | 8 ++++++++
 qapi/block-core.json   | 5 ++++-
 util/hbitmap.c         | 8 ++++++++
 4 files changed, 21 insertions(+), 1 deletion(-)

Comments

Max Reitz Oct. 10, 2016, 4:44 p.m. UTC | #1
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c   | 1 +
>  include/qemu/hbitmap.h | 8 ++++++++
>  qapi/block-core.json   | 5 ++++-
>  util/hbitmap.c         | 8 ++++++++
>  4 files changed, 21 insertions(+), 1 deletion(-)

Having read John's and Eric's comments, I won't block this patch, but I
won't give an R-b either.

It's probably true that this will not significantly slow down the
query-block call, but doing this only for debugging does not seem right
to me.

I'm not sure what the right way would be to get this information out
(...maybe make it optional and set it only if qtest_enabled() is true?),
but in my opinion this is not the right way.

Since I'm not the maintainer of the bitmap code (Fam and John are, even
though their MAINTAINERS patch is not in master still...), I can't and
won't block this, though.

Max
Max Reitz Oct. 10, 2016, 5:03 p.m. UTC | #2
On 10.10.2016 18:44, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block/dirty-bitmap.c   | 1 +
>>  include/qemu/hbitmap.h | 8 ++++++++
>>  qapi/block-core.json   | 5 ++++-
>>  util/hbitmap.c         | 8 ++++++++
>>  4 files changed, 21 insertions(+), 1 deletion(-)
> 
> Having read John's and Eric's comments, I won't block this patch, but I
> won't give an R-b either.
> 
> It's probably true that this will not significantly slow down the
> query-block call, but doing this only for debugging does not seem right
> to me.
> 
> I'm not sure what the right way would be to get this information out
> (...maybe make it optional and set it only if qtest_enabled() is true?),
> but in my opinion this is not the right way.

By the way, the cleanest way I can come up with (which I didn't write
about in my first reply because it's not so trivial) would be some kind
of debugging QMP command convention. For instance, we could say that all
debugging commands have an x-debug- prefix, and then you could add an
x-debug-get-bitmap-md5 to read the MD5 hash of a named bitmap. That
would appear to be the cleanest way to do this to me.

Max

> Since I'm not the maintainer of the bitmap code (Fam and John are, even
> though their MAINTAINERS patch is not in master still...), I can't and
> won't block this, though.
> 
> Max
>
Eric Blake Oct. 10, 2016, 7:22 p.m. UTC | #3
On 10/10/2016 12:03 PM, Max Reitz wrote:
>> I'm not sure what the right way would be to get this information out
>> (...maybe make it optional and set it only if qtest_enabled() is true?),
>> but in my opinion this is not the right way.
> 
> By the way, the cleanest way I can come up with (which I didn't write
> about in my first reply because it's not so trivial) would be some kind
> of debugging QMP command convention. For instance, we could say that all
> debugging commands have an x-debug- prefix, and then you could add an
> x-debug-get-bitmap-md5 to read the MD5 hash of a named bitmap. That
> would appear to be the cleanest way to do this to me.

Anything named x- is automatically not stable, and therefore free to use
in the testsuite without having to worry about keeping it backwards
compatible (libvirt won't touch x- commands).  The suggestion of
x-debug- to mark it as specifically for debug use is reasonable.

Patch
diff mbox

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 392d660..d0fe382 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -359,6 +359,7 @@  BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
         info->status = bdrv_dirty_bitmap_status(bm);
+        info->md5 = hbitmap_md5(bm->bitmap);
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 64e5963..a47ce6a 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -240,6 +240,14 @@  void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
 void hbitmap_deserialize_finish(HBitmap *hb);
 
 /**
+ * hbitmap_md5:
+ * @bitmap: HBitmap to operate on.
+ *
+ * Returns md5 checksum of the last level.
+ */
+char *hbitmap_md5(const HBitmap *bitmap);
+
+/**
  * hbitmap_free:
  * @hb: HBitmap to operate on.
  *
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 087a681..8387e9c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -419,11 +419,14 @@ 
 #
 # @status: current status of the dirty bitmap (since 2.4)
 #
+# @md5: md5 checksum (as a hexadecimal string) of the last bitmap level
+#       (since 2.8)
+#
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
-           'status': 'DirtyBitmapStatus'} }
+           'status': 'DirtyBitmapStatus', 'md5': 'str'} }
 
 ##
 # @BlockInfo:
diff --git a/util/hbitmap.c b/util/hbitmap.c
index f49520f..1f0c417 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -707,3 +707,11 @@  void hbitmap_free_meta(HBitmap *hb)
     hbitmap_free(hb->meta);
     hb->meta = NULL;
 }
+
+char *hbitmap_md5(const HBitmap *bitmap)
+{
+    uint64_t size =
+        MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+    const guchar *data = (const guchar *)bitmap->levels[HBITMAP_LEVELS - 1];
+    return g_compute_checksum_for_data(G_CHECKSUM_MD5, data, size);
+}