Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/dirty-bitmap.c | 5 +++++
blockdev.c | 25 +++++++++++++++++++++++++
include/block/dirty-bitmap.h | 1 +
include/qemu/hbitmap.h | 8 ++++++++
qapi/block-core.json | 27 +++++++++++++++++++++++++++
tests/Makefile.include | 2 +-
util/hbitmap.c | 11 +++++++++++
7 files changed, 78 insertions(+), 1 deletion(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d1469418e6..5fcf917707 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -725,3 +725,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
QLIST_NEXT(bitmap, list);
}
+
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
+{
+ return hbitmap_sha256(bitmap->bitmap, errp);
+}
diff --git a/blockdev.c b/blockdev.c
index 4bb7033994..3c8fb75208 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2832,6 +2832,31 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
bdrv_clear_dirty_bitmap(bitmap, NULL);
}
+BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
+ const char *name,
+ Error **errp)
+{
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+ BlockDirtyBitmapSha256 *ret = NULL;
+ char *sha256;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap || !bs) {
+ return NULL;
+ }
+
+ sha256 = bdrv_dirty_bitmap_sha256(bitmap, errp);
+ if (sha256 == NULL) {
+ return NULL;
+ }
+
+ ret = g_new(BlockDirtyBitmapSha256, 1);
+ ret->sha256 = sha256;
+
+ return ret;
+}
+
void hmp_drive_del(Monitor *mon, const QDict *qdict)
{
const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index ccf2f81640..744479bc76 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -97,5 +97,6 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap);
+char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
#endif
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index b52304ac29..d3a74a21fc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -253,6 +253,14 @@ void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count,
void hbitmap_deserialize_finish(HBitmap *hb);
/**
+ * hbitmap_sha256:
+ * @bitmap: HBitmap to operate on.
+ *
+ * Returns SHA256 hash of the last level.
+ */
+char *hbitmap_sha256(const HBitmap *bitmap, Error **errp);
+
+/**
* hbitmap_free:
* @hb: HBitmap to operate on.
*
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5c42cc7790..6ad8585400 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1644,6 +1644,33 @@
'data': 'BlockDirtyBitmap' }
##
+# @BlockDirtyBitmapSha256:
+#
+# SHA256 hash of dirty bitmap data
+#
+# @sha256: ASCII representation of SHA256 bitmap hash
+#
+# Since: 2.10
+##
+ { 'struct': 'BlockDirtyBitmapSha256',
+ 'data': {'sha256': 'str'} }
+
+##
+# @x-debug-block-dirty-bitmap-sha256:
+#
+# Get bitmap SHA256
+#
+# Returns: BlockDirtyBitmapSha256 on success
+# If @node is not a valid block device, DeviceNotFound
+# If @name is not found or if hashing has failed, GenericError with an
+# explanation
+#
+# Since: 2.10
+##
+ { 'command': 'x-debug-block-dirty-bitmap-sha256',
+ 'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
+
+##
# @blockdev-mirror:
#
# Start mirroring a block device's writes to a new destination.
diff --git a/tests/Makefile.include b/tests/Makefile.include
index ae889cae02..c738e92673 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -553,7 +553,7 @@ tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-u
tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
-tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
+tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o migration/page_cache.o $(test-util-obj-y)
tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 0c1591a594..21535cc90b 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -13,6 +13,7 @@
#include "qemu/hbitmap.h"
#include "qemu/host-utils.h"
#include "trace.h"
+#include "crypto/hash.h"
/* HBitmaps provides an array of bits. The bits are stored as usual in an
* array of unsigned longs, but HBitmap is also optimized to provide fast
@@ -727,3 +728,13 @@ void hbitmap_free_meta(HBitmap *hb)
hbitmap_free(hb->meta);
hb->meta = NULL;
}
+
+char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)
+{
+ size_t size = bitmap->sizes[HBITMAP_LEVELS - 1] * sizeof(unsigned long);
+ char *data = (char *)bitmap->levels[HBITMAP_LEVELS - 1];
+ char *hash = NULL;
+ qcrypto_hash_digest(QCRYPTO_HASH_ALG_SHA256, data, size, &hash, errp);
+
+ return hash;
+}
--
2.11.1
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/dirty-bitmap.c | 5 +++++
> blockdev.c | 25 +++++++++++++++++++++++++
> include/block/dirty-bitmap.h | 1 +
> include/qemu/hbitmap.h | 8 ++++++++
> qapi/block-core.json | 27 +++++++++++++++++++++++++++
> tests/Makefile.include | 2 +-
> util/hbitmap.c | 11 +++++++++++
> 7 files changed, 78 insertions(+), 1 deletion(-)
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5c42cc7790..6ad8585400 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1644,6 +1644,33 @@
> 'data': 'BlockDirtyBitmap' }
>
> ##
> +# @BlockDirtyBitmapSha256:
> +#
> +# SHA256 hash of dirty bitmap data
> +#
> +# @sha256: ASCII representation of SHA256 bitmap hash
Spell it SHA-256, please. The member name @sha256 can stay.
SHA-256 is 256 binary bits. Please specify how they are represented in
ASCII. It better be base64 (RFC 4648), because we use that elsewhere.
> +#
> +# Since: 2.10
> +##
> + { 'struct': 'BlockDirtyBitmapSha256',
> + 'data': {'sha256': 'str'} }
> +
> +##
> +# @x-debug-block-dirty-bitmap-sha256:
> +#
> +# Get bitmap SHA256
> +#
> +# Returns: BlockDirtyBitmapSha256 on success
> +# If @node is not a valid block device, DeviceNotFound
> +# If @name is not found or if hashing has failed, GenericError with an
> +# explanation
Please wrap your comment lines around column 70.
> +#
> +# Since: 2.10
> +##
> + { 'command': 'x-debug-block-dirty-bitmap-sha256',
> + 'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
> +
> +##
> # @blockdev-mirror:
> #
> # Start mirroring a block device's writes to a new destination.
[...]
On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > --- > > block/dirty-bitmap.c | 5 +++++ > > blockdev.c | 25 +++++++++++++++++++++++++ > > include/block/dirty-bitmap.h | 1 + > > include/qemu/hbitmap.h | 8 ++++++++ > > qapi/block-core.json | 27 +++++++++++++++++++++++++++ > > tests/Makefile.include | 2 +- > > util/hbitmap.c | 11 +++++++++++ > > 7 files changed, 78 insertions(+), 1 deletion(-) > [...] > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index 5c42cc7790..6ad8585400 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -1644,6 +1644,33 @@ > > 'data': 'BlockDirtyBitmap' } > > > > ## > > +# @BlockDirtyBitmapSha256: > > +# > > +# SHA256 hash of dirty bitmap data > > +# > > +# @sha256: ASCII representation of SHA256 bitmap hash > > Spell it SHA-256, please. The member name @sha256 can stay. > > SHA-256 is 256 binary bits. Please specify how they are represented in > ASCII. It better be base64 (RFC 4648), because we use that elsewhere. It is filled later in this patch using qcrypto_hash_digest, so it is just a hex string representing the hash, not base64. For the latter you can use qcrypto_hash_base64 Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >> >> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> > --- >> > block/dirty-bitmap.c | 5 +++++ >> > blockdev.c | 25 +++++++++++++++++++++++++ >> > include/block/dirty-bitmap.h | 1 + >> > include/qemu/hbitmap.h | 8 ++++++++ >> > qapi/block-core.json | 27 +++++++++++++++++++++++++++ >> > tests/Makefile.include | 2 +- >> > util/hbitmap.c | 11 +++++++++++ >> > 7 files changed, 78 insertions(+), 1 deletion(-) >> [...] >> > diff --git a/qapi/block-core.json b/qapi/block-core.json >> > index 5c42cc7790..6ad8585400 100644 >> > --- a/qapi/block-core.json >> > +++ b/qapi/block-core.json >> > @@ -1644,6 +1644,33 @@ >> > 'data': 'BlockDirtyBitmap' } >> > >> > ## >> > +# @BlockDirtyBitmapSha256: >> > +# >> > +# SHA256 hash of dirty bitmap data >> > +# >> > +# @sha256: ASCII representation of SHA256 bitmap hash >> >> Spell it SHA-256, please. The member name @sha256 can stay. >> >> SHA-256 is 256 binary bits. Please specify how they are represented in >> ASCII. It better be base64 (RFC 4648), because we use that elsewhere. > > It is filled later in this patch using qcrypto_hash_digest, so it is just > a hex string representing the hash, not base64. For the latter you can > use qcrypto_hash_base64 I got two points: 1. Whatever encoding we use, it needs to be documented. 2. The fewer binary -> ASCII encodings we use, the better. We already use base64.
07.07.2017 12:00, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > >> On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/dirty-bitmap.c | 5 +++++ >>>> blockdev.c | 25 +++++++++++++++++++++++++ >>>> include/block/dirty-bitmap.h | 1 + >>>> include/qemu/hbitmap.h | 8 ++++++++ >>>> qapi/block-core.json | 27 +++++++++++++++++++++++++++ >>>> tests/Makefile.include | 2 +- >>>> util/hbitmap.c | 11 +++++++++++ >>>> 7 files changed, 78 insertions(+), 1 deletion(-) >>> [...] >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index 5c42cc7790..6ad8585400 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -1644,6 +1644,33 @@ >>>> 'data': 'BlockDirtyBitmap' } >>>> >>>> ## >>>> +# @BlockDirtyBitmapSha256: >>>> +# >>>> +# SHA256 hash of dirty bitmap data >>>> +# >>>> +# @sha256: ASCII representation of SHA256 bitmap hash >>> Spell it SHA-256, please. The member name @sha256 can stay. >>> >>> SHA-256 is 256 binary bits. Please specify how they are represented in >>> ASCII. It better be base64 (RFC 4648), because we use that elsewhere. >> It is filled later in this patch using qcrypto_hash_digest, so it is just >> a hex string representing the hash, not base64. For the latter you can >> use qcrypto_hash_base64 > I got two points: > > 1. Whatever encoding we use, it needs to be documented. > > 2. The fewer binary -> ASCII encodings we use, the better. We already > use base64. ASCII format for check sum is more common as it is more readable. It is used in the internet to check downloads, it is used by standard utility sha256sum. So, it may be better for the monitor. However, if it is needed, I can make a follow-up patch, it is very easy, just s/qcrypto_hash_digest/qcrypto_hash_base64/ in util/hbitmap.c. iotest 165 - the only user of the feature - doesn't need any changes. -- Best regards, Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 07.07.2017 12:00, Markus Armbruster wrote: >> "Daniel P. Berrange" <berrange@redhat.com> writes: >> >>> On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: >>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> block/dirty-bitmap.c | 5 +++++ >>>>> blockdev.c | 25 +++++++++++++++++++++++++ >>>>> include/block/dirty-bitmap.h | 1 + >>>>> include/qemu/hbitmap.h | 8 ++++++++ >>>>> qapi/block-core.json | 27 +++++++++++++++++++++++++++ >>>>> tests/Makefile.include | 2 +- >>>>> util/hbitmap.c | 11 +++++++++++ >>>>> 7 files changed, 78 insertions(+), 1 deletion(-) >>>> [...] >>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>>> index 5c42cc7790..6ad8585400 100644 >>>>> --- a/qapi/block-core.json >>>>> +++ b/qapi/block-core.json >>>>> @@ -1644,6 +1644,33 @@ >>>>> 'data': 'BlockDirtyBitmap' } >>>>> ## >>>>> +# @BlockDirtyBitmapSha256: >>>>> +# >>>>> +# SHA256 hash of dirty bitmap data >>>>> +# >>>>> +# @sha256: ASCII representation of SHA256 bitmap hash >>>> Spell it SHA-256, please. The member name @sha256 can stay. >>>> >>>> SHA-256 is 256 binary bits. Please specify how they are represented in >>>> ASCII. It better be base64 (RFC 4648), because we use that elsewhere. >>> It is filled later in this patch using qcrypto_hash_digest, so it is just >>> a hex string representing the hash, not base64. For the latter you can >>> use qcrypto_hash_base64 >> I got two points: >> >> 1. Whatever encoding we use, it needs to be documented. >> >> 2. The fewer binary -> ASCII encodings we use, the better. We already >> use base64. > > > ASCII format for check sum is more common as it is more readable. It > is used in the internet to check downloads, it is used by standard > utility sha256sum. So, it may be better for the monitor. > > However, if it is needed, I can make a follow-up patch, it is very > easy, just s/qcrypto_hash_digest/qcrypto_hash_base64/ in > util/hbitmap.c. iotest 165 - the only user of the feature - doesn't > need any changes. If the is a standard way to represent SHA-256 in ASCII, use it. Whatever you use, document it clearly in the QAPI schema.
On 07/07/2017 09:53 AM, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> 07.07.2017 12:00, Markus Armbruster wrote: >>> "Daniel P. Berrange" <berrange@redhat.com> writes: >>> >>>> On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: >>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>>> --- >>>>>> block/dirty-bitmap.c | 5 +++++ >>>>>> blockdev.c | 25 +++++++++++++++++++++++++ >>>>>> include/block/dirty-bitmap.h | 1 + >>>>>> include/qemu/hbitmap.h | 8 ++++++++ >>>>>> qapi/block-core.json | 27 +++++++++++++++++++++++++++ >>>>>> tests/Makefile.include | 2 +- >>>>>> util/hbitmap.c | 11 +++++++++++ >>>>>> 7 files changed, 78 insertions(+), 1 deletion(-) >>>>> [...] >>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>>>> index 5c42cc7790..6ad8585400 100644 >>>>>> --- a/qapi/block-core.json >>>>>> +++ b/qapi/block-core.json >>>>>> @@ -1644,6 +1644,33 @@ >>>>>> 'data': 'BlockDirtyBitmap' } >>>>>> ## >>>>>> +# @BlockDirtyBitmapSha256: >>>>>> +# >>>>>> +# SHA256 hash of dirty bitmap data >>>>>> +# >>>>>> +# @sha256: ASCII representation of SHA256 bitmap hash >>>>> Spell it SHA-256, please. The member name @sha256 can stay. >>>>> >>>>> SHA-256 is 256 binary bits. Please specify how they are represented in >>>>> ASCII. It better be base64 (RFC 4648), because we use that elsewhere. >>>> It is filled later in this patch using qcrypto_hash_digest, so it is just >>>> a hex string representing the hash, not base64. For the latter you can >>>> use qcrypto_hash_base64 >>> I got two points: >>> >>> 1. Whatever encoding we use, it needs to be documented. >>> >>> 2. The fewer binary -> ASCII encodings we use, the better. We already >>> use base64. >> >> >> ASCII format for check sum is more common as it is more readable. It >> is used in the internet to check downloads, it is used by standard >> utility sha256sum. So, it may be better for the monitor. >> >> However, if it is needed, I can make a follow-up patch, it is very >> easy, just s/qcrypto_hash_digest/qcrypto_hash_base64/ in >> util/hbitmap.c. iotest 165 - the only user of the feature - doesn't >> need any changes. > > If the is a standard way to represent SHA-256 in ASCII, use it. > > Whatever you use, document it clearly in the QAPI schema. > I... should we, though? It's a debug interface for testing only, basically. Couldn't think of a better way to test it, and people demanded tests. How's this for documentation: "The hash will be in an arbitrary format that changes every time you look away from this specification. Any similarity, real or imagined, to a canonical SHA-256 ASCII string is purely coincidental." Basically, I would actually rather go out of my way to obfuscate this command, not document it... Maybe that's wrong-headed of me, but I still maintain that it's not terribly important because I'd rather people never, ever try to use this in production.
John Snow <jsnow@redhat.com> writes: > On 07/07/2017 09:53 AM, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >> >>> 07.07.2017 12:00, Markus Armbruster wrote: >>>> "Daniel P. Berrange" <berrange@redhat.com> writes: >>>> >>>>> On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: >>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>>>>> >>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>>>> --- >>>>>>> block/dirty-bitmap.c | 5 +++++ >>>>>>> blockdev.c | 25 +++++++++++++++++++++++++ >>>>>>> include/block/dirty-bitmap.h | 1 + >>>>>>> include/qemu/hbitmap.h | 8 ++++++++ >>>>>>> qapi/block-core.json | 27 +++++++++++++++++++++++++++ >>>>>>> tests/Makefile.include | 2 +- >>>>>>> util/hbitmap.c | 11 +++++++++++ >>>>>>> 7 files changed, 78 insertions(+), 1 deletion(-) >>>>>> [...] >>>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>>>>> index 5c42cc7790..6ad8585400 100644 >>>>>>> --- a/qapi/block-core.json >>>>>>> +++ b/qapi/block-core.json >>>>>>> @@ -1644,6 +1644,33 @@ >>>>>>> 'data': 'BlockDirtyBitmap' } >>>>>>> ## >>>>>>> +# @BlockDirtyBitmapSha256: >>>>>>> +# >>>>>>> +# SHA256 hash of dirty bitmap data >>>>>>> +# >>>>>>> +# @sha256: ASCII representation of SHA256 bitmap hash >>>>>> Spell it SHA-256, please. The member name @sha256 can stay. >>>>>> >>>>>> SHA-256 is 256 binary bits. Please specify how they are represented in >>>>>> ASCII. It better be base64 (RFC 4648), because we use that elsewhere. >>>>> It is filled later in this patch using qcrypto_hash_digest, so it is just >>>>> a hex string representing the hash, not base64. For the latter you can >>>>> use qcrypto_hash_base64 >>>> I got two points: >>>> >>>> 1. Whatever encoding we use, it needs to be documented. >>>> >>>> 2. The fewer binary -> ASCII encodings we use, the better. We already >>>> use base64. >>> >>> >>> ASCII format for check sum is more common as it is more readable. It >>> is used in the internet to check downloads, it is used by standard >>> utility sha256sum. So, it may be better for the monitor. >>> >>> However, if it is needed, I can make a follow-up patch, it is very >>> easy, just s/qcrypto_hash_digest/qcrypto_hash_base64/ in >>> util/hbitmap.c. iotest 165 - the only user of the feature - doesn't >>> need any changes. >> >> If the is a standard way to represent SHA-256 in ASCII, use it. >> >> Whatever you use, document it clearly in the QAPI schema. >> > > I... should we, though? It's a debug interface for testing only, > basically. Couldn't think of a better way to test it, and people > demanded tests. > > How's this for documentation: > > "The hash will be in an arbitrary format that changes every time you > look away from this specification. Any similarity, real or imagined, to > a canonical SHA-256 ASCII string is purely coincidental." > > Basically, I would actually rather go out of my way to obfuscate this > command, not document it... > > Maybe that's wrong-headed of me, but I still maintain that it's not > terribly important because I'd rather people never, ever try to use this > in production. It is wrong-headed of you :) We mark stuff not covered by QMP's compatibility promise with x- prefixes, not with incomplete documentation, and not by obsfuscating it. People trying to use this in non-production need to know how it works, too. If you think people need to be scared away some more, feel free to write something like "currently SHA-256 ASCII". If that's not enough of a hint for you, you might add "but you shouldn't rely on that".
On 07/10/2017 02:49 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 07/07/2017 09:53 AM, Markus Armbruster wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>> >>>> 07.07.2017 12:00, Markus Armbruster wrote: >>>>> "Daniel P. Berrange" <berrange@redhat.com> writes: >>>>> >>>>>> On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: >>>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>>>>>> >>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>>>>> --- >>>>>>>> block/dirty-bitmap.c | 5 +++++ >>>>>>>> blockdev.c | 25 +++++++++++++++++++++++++ >>>>>>>> include/block/dirty-bitmap.h | 1 + >>>>>>>> include/qemu/hbitmap.h | 8 ++++++++ >>>>>>>> qapi/block-core.json | 27 +++++++++++++++++++++++++++ >>>>>>>> tests/Makefile.include | 2 +- >>>>>>>> util/hbitmap.c | 11 +++++++++++ >>>>>>>> 7 files changed, 78 insertions(+), 1 deletion(-) >>>>>>> [...] >>>>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>>>>>> index 5c42cc7790..6ad8585400 100644 >>>>>>>> --- a/qapi/block-core.json >>>>>>>> +++ b/qapi/block-core.json >>>>>>>> @@ -1644,6 +1644,33 @@ >>>>>>>> 'data': 'BlockDirtyBitmap' } >>>>>>>> ## >>>>>>>> +# @BlockDirtyBitmapSha256: >>>>>>>> +# >>>>>>>> +# SHA256 hash of dirty bitmap data >>>>>>>> +# >>>>>>>> +# @sha256: ASCII representation of SHA256 bitmap hash >>>>>>> Spell it SHA-256, please. The member name @sha256 can stay. >>>>>>> >>>>>>> SHA-256 is 256 binary bits. Please specify how they are represented in >>>>>>> ASCII. It better be base64 (RFC 4648), because we use that elsewhere. >>>>>> It is filled later in this patch using qcrypto_hash_digest, so it is just >>>>>> a hex string representing the hash, not base64. For the latter you can >>>>>> use qcrypto_hash_base64 >>>>> I got two points: >>>>> >>>>> 1. Whatever encoding we use, it needs to be documented. >>>>> >>>>> 2. The fewer binary -> ASCII encodings we use, the better. We already >>>>> use base64. >>>> >>>> >>>> ASCII format for check sum is more common as it is more readable. It >>>> is used in the internet to check downloads, it is used by standard >>>> utility sha256sum. So, it may be better for the monitor. >>>> >>>> However, if it is needed, I can make a follow-up patch, it is very >>>> easy, just s/qcrypto_hash_digest/qcrypto_hash_base64/ in >>>> util/hbitmap.c. iotest 165 - the only user of the feature - doesn't >>>> need any changes. >>> >>> If the is a standard way to represent SHA-256 in ASCII, use it. >>> >>> Whatever you use, document it clearly in the QAPI schema. >>> >> >> I... should we, though? It's a debug interface for testing only, >> basically. Couldn't think of a better way to test it, and people >> demanded tests. >> >> How's this for documentation: >> >> "The hash will be in an arbitrary format that changes every time you >> look away from this specification. Any similarity, real or imagined, to >> a canonical SHA-256 ASCII string is purely coincidental." >> >> Basically, I would actually rather go out of my way to obfuscate this >> command, not document it... >> >> Maybe that's wrong-headed of me, but I still maintain that it's not >> terribly important because I'd rather people never, ever try to use this >> in production. > > It is wrong-headed of you :) > I figured so, but I wanted to be very facetiously clear that we didn't really intend for this to be used outside of our own testing suite. I'd love to not even have it be a real QMP interface available in non-testing binaries at all, but that gets weird quickly. Sorry for being a wise-ass. > We mark stuff not covered by QMP's compatibility promise with x- > prefixes, not with incomplete documentation, and not by obsfuscating it. > People trying to use this in non-production need to know how it works, > too. > > If you think people need to be scared away some more, feel free to write > something like "currently SHA-256 ASCII". If that's not enough of a > hint for you, you might add "but you shouldn't rely on that". > I'd still like to decorate it with some little emoji skulls, too. ☠☠💀💀 Make sure people know it's a very bad idea to use it. Maybe QEMU can do some wall text whenever it gets used that says "Someone is doing something very naughty when we asked very nicely for them not to." Kidding again, of course, but just trying to emphasize that this command literally serves no purpose other than for testing and is not expected to be used apart from that. The format could be changed if you desired, but as this has been on the list for over a year, I'd have to unfortunately request that you make that change if it was important to you. --js
On 07/07/2017 03:13 AM, Daniel P. Berrange wrote: > On Fri, Jul 07, 2017 at 10:05:22AM +0200, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/dirty-bitmap.c | 5 +++++ >>> blockdev.c | 25 +++++++++++++++++++++++++ >>> include/block/dirty-bitmap.h | 1 + >>> include/qemu/hbitmap.h | 8 ++++++++ >>> qapi/block-core.json | 27 +++++++++++++++++++++++++++ >>> tests/Makefile.include | 2 +- >>> util/hbitmap.c | 11 +++++++++++ >>> 7 files changed, 78 insertions(+), 1 deletion(-) >> [...] >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 5c42cc7790..6ad8585400 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -1644,6 +1644,33 @@ >>> 'data': 'BlockDirtyBitmap' } >>> >>> ## >>> +# @BlockDirtyBitmapSha256: >>> +# >>> +# SHA256 hash of dirty bitmap data >>> +# >>> +# @sha256: ASCII representation of SHA256 bitmap hash >> >> Spell it SHA-256, please. The member name @sha256 can stay. >> >> SHA-256 is 256 binary bits. Please specify how they are represented in >> ASCII. It better be base64 (RFC 4648), because we use that elsewhere. > > It is filled later in this patch using qcrypto_hash_digest, so it is just > a hex string representing the hash, not base64. For the latter you can > use qcrypto_hash_base64 hex (aka base16) is used for displaying UUIDs across QMP. Furthermore, this is an x- command, useful only for debugging, so we can feel free to change the underlying implementation without breaking clients (if we prefer to go with the shorter base64 string instead of the longer hex string, even if we make that switch after a release). Still, it's nice to avoid churn, and Markus does have a point about reusing a common base64 theme for displaying arbitrary binary data, rather than having ad hoc differences. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
© 2016 - 2026 Red Hat, Inc.