Re: [Qemu-devel] [PATCH 15/18] qapi: add md5 checksum of last dirty bitmap level to query-block

[Daniel P. Berrange]

On Tue, Aug 16, 2016 at 11:37:28AM +0100, Daniel P. Berrange wrote:
> On Tue, Aug 16, 2016 at 01:26:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Reviewed-by: John Snow <address@hidden>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >  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(-)
> > 
> > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > index 32aa6eb..f895fce 100644
> > --- a/block/dirty-bitmap.c
> > +++ b/block/dirty-bitmap.c
> > @@ -365,6 +365,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 eb46475..8d4f836 100644
> > --- a/include/qemu/hbitmap.h
> > +++ b/include/qemu/hbitmap.h
> > @@ -225,6 +225,14 @@ void hbitmap_deserialize_zeroes(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 2bbc027..5642a8a 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.6)
> > +#
> >  # 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 6a13c12..4afb3d5 100644
> > --- a/util/hbitmap.c
> > +++ b/util/hbitmap.c
> > @@ -669,3 +669,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);
> > +}
> 
> Please use one of the qcrypto_hash_* functions, not GChecksum directly,
> as this ensures we use the hash impls that have been through crypto
> certification when needed by people shipping QEM.

Oh and why MD5 here. Can we use a modern strong algorithm SHA256 instead
of MD5 which is known to be susceptible to collisions

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|