Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Dec 10, 2018 at 03:06:59PM +0100, Alberto Garcia wrote:
> On Fri 07 Dec 2018 05:13:51 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> > @@ -148,12 +154,97 @@ int qcrypto_block_encrypt(QCryptoBlock *block,
> >  
> >  QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
> >  {
> > -    return block->cipher;
> > +    /* Ciphers should be accessed through pop/push method to be thread-safe.
> > +     * Better, they should not be accessed externally at all (note, that
> > +     * pop/push are static functions)
> > +     * This function is used only in test with one thread (it's safe to skip
> > +     * pop/push interface), so it's enough to assert it here:
> > +     */
> > +    assert(block->n_ciphers <= 1);
> > +    return block->ciphers ? block->ciphers[0] : NULL;
> 
> If this is only supposed to be called in test mode I think you can also
> assert that g_test_initialized() is true.
> 
> And the same with qcrypto_block_get_ivgen() later in this patch.

I consider these APIs as being valid for use anywhere - it just
happens it is only used in the tests right now. So I think it is
ok to assert on n_cipers here.

> > +int qcrypto_block_init_cipher(QCryptoBlock *block,
> > +                              QCryptoCipherAlgorithm alg,
> > +                              QCryptoCipherMode mode,
> > +                              const uint8_t *key, size_t nkey,
> > +                              size_t n_threads, Error **errp)
> > +{
> > +    size_t i;
> > +
> > +    assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
> > +
> > +    block->ciphers = g_new0(QCryptoCipher *, n_threads);
> 
> You can use g_new() instead of g_new0() because you're anyway
> overwriting all elements of the array.

I'd rather have it initialized to zero upfront, so if creating any
cipher in the array fails, we don't have uninitialized array elements
during later cleanup code.

> But these are minor nits, the patchs looks good to me else.
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>

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