Re: [PATCH 2/2] tests/unit: always build the pbkdf crypto unit test

[Daniel P . Berrangé]

On Mon, Sep 02, 2024 at 09:45:04PM +0200, Philippe Mathieu-Daudé wrote:
> On 30/8/24 13:05, Daniel P. Berrangé wrote:
> > The meson rules were excluding the pbkdf crypto test when gnutls was the
> > crypto backend. It was then excluded again in #if statements in the test
> > file.
> > 
> > Rather than update these conditions, remove them all, and use the result
> > of the qcrypto_pbkdf_supports() function to determine whether to skip
> > test registration.
> > 
> > Also add CONFIG_DARWIN to the remaining condition, since we have a way
> > to measure CPU time on this platform since commit bf98afc75efedf1.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/unit/meson.build         | 4 +---
> >   tests/unit/test-crypto-pbkdf.c | 9 ++++++---
> >   2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> > index 490ab8182d..972d792883 100644
> > --- a/tests/unit/meson.build
> > +++ b/tests/unit/meson.build
> > @@ -121,9 +121,7 @@ if have_block
> >     if config_host_data.get('CONFIG_REPLICATION')
> >       tests += {'test-replication': [testblock]}
> >     endif
> > -  if nettle.found() or gcrypt.found()
> > -    tests += {'test-crypto-pbkdf': [io]}
> > -  endif
> > +  tests += {'test-crypto-pbkdf': [io]}
> >   endif
> >   if have_system
> > diff --git a/tests/unit/test-crypto-pbkdf.c b/tests/unit/test-crypto-pbkdf.c
> > index 43c417f6b4..034bb02422 100644
> > --- a/tests/unit/test-crypto-pbkdf.c
> > +++ b/tests/unit/test-crypto-pbkdf.c
> > @@ -25,8 +25,7 @@
> >   #include <sys/resource.h>
> >   #endif
> > -#if ((defined(CONFIG_NETTLE) || defined(CONFIG_GCRYPT)) && \
> > -     (defined(_WIN32) || defined(RUSAGE_THREAD)))
> > +#if defined(_WIN32) || defined(RUSAGE_THREAD) || defined(CONFIG_DARWIN)
> 
> Add CONFIG_DARWIN in a subsequent commit?

Yes, classic trap. If you have "Also ..." in a commit message then you've
just told yourself it should have been a separate commit :-)

> 
> >   #include "crypto/pbkdf.h"
> >   typedef struct QCryptoPbkdfTestData QCryptoPbkdfTestData;
> > @@ -422,13 +421,17 @@ int main(int argc, char **argv)
> >       g_assert(qcrypto_init(NULL) == 0);
> >       for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
> > +        if (!qcrypto_pbkdf2_supports(test_data[i].hash)) {
> > +            continue;
> > +        }
> > +
> >           if (!test_data[i].slow ||
> >               g_test_slow()) {
> >               g_test_add_data_func(test_data[i].path, &test_data[i], 
> > test_pbkdf);
> >           }
> >       }
> > -    if (g_test_slow()) {
> > +    if (g_test_slow() && qcrypto_pbkdf2_supports(QCRYPTO_HASH_ALG_SHA256)) 
> > {
> >           g_test_add_func("/crypt0/pbkdf/timing", test_pbkdf_timing);
> 
> While here, rename test_pbkdf_timing -> test_pbkdf_sha256_timing?

Will do

> 
> >       }
> 
> Anyway,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

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