Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/2] crypto: use glib as fallback for hash algorithm

[Daniel P. Berrange]

On Tue, Jul 05, 2016 at 09:03:26AM -0600, Eric Blake wrote:
> On 07/05/2016 04:49 AM, Daniel P. Berrange wrote:
> > GLib >= 2.16 provides GChecksum API which is good enough
> > for md5, sha1, sha256 and sha512. Use this as a final
> > fallback if neither nettle or gcrypt are available. This
> > lets us remove the stub hash impl, and so callers can
> > be sure those 4 algs are always available at compile
> > time. They may still be disabled at runtime, so a check
> > for qcrypto_hash_supports() is still best practice to
> > report good error messages.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  crypto/Makefile.objs |  2 +-
> >  crypto/hash-glib.c   | 94 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  crypto/hash-stub.c   | 41 -----------------------
> >  3 files changed, 95 insertions(+), 42 deletions(-)
> >  create mode 100644 crypto/hash-glib.c
> >  delete mode 100644 crypto/hash-stub.c
> > 
> 
> > +gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg)
> > +{
> > +    if (alg < G_N_ELEMENTS(qcrypto_hash_alg_map) &&
> > +        qcrypto_hash_alg_map[alg] != -1) {
> > +        return true;
> > +    }
> > +    return false;
> > +}
> > +
> > +
> > +int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg,
> > +                        const struct iovec *iov,
> > +                        size_t niov,
> > +                        uint8_t **result,
> > +                        size_t *resultlen,
> > +                        Error **errp)
> > +{
> > +    int i, ret;
> > +    GChecksum *cs;
> > +
> > +    if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_map) ||
> > +        qcrypto_hash_alg_map[alg] == -1) {
> 
> Worth writing this as 'if (!gcrypto_hash_supports(alg)) {' ?

This pattern is used in the nettle + gcrypt impls too. I'd be happy to
switch to what you suggest in all impls separately.

> Otherwise,
> Reviewed-by: Eric Blake <address@hidden>

Oh, and BTW the pre-existing test-crypto-hash unit tests will already
provide coverage for this implementation & i've checked it passes
when --disable-nettle --disable-gcrypt are given to confnigure.



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