Re: [Qemu-block] [PATCH v5 09/18] qcow: convert QCow to use QCryptoBlock for encryption

[Daniel P. Berrange]

On Tue, Feb 21, 2017 at 02:19:46PM +0100, Alberto Garcia wrote:
> On Tue 21 Feb 2017 12:55:03 PM CET, Daniel P. Berrange wrote:
> > @@ -175,8 +185,31 @@ static int qcow_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >              ret = -ENOSYS;
> >              goto fail;
> >          }
> > +        if (s->crypt_method_header == QCOW_CRYPT_AES) {
> > +            crypto_opts = block_crypto_open_opts_init(
> > +                Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, "aes-", &local_err);
> > +            if (local_err) {
> > +                error_propagate(errp, local_err);
> > +                ret = -EINVAL;
> > +                goto fail;
> > +            }
> >
> > +            if (flags & BDRV_O_NO_IO) {
> > +                cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> > +            }
> > +            s->crypto = qcrypto_block_open(crypto_opts, NULL, NULL,
> > +                                           cflags, errp);
> 
> You don't call qcrypto_block_free() if qcow_open() eventually fails.
> Although qcow_close() takes care of that, a failure to open the image
> sets bs->drv = NULL in bdrv_open_common(), preventing qcow_close() from
> being called.

Yes, I'll fix that leak.

> 
> > @@ -260,14 +293,17 @@ static int qcow_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >          goto fail;
> >      }
> >  
> > +    qapi_free_QCryptoBlockOpenOptions(crypto_opts);
> >      qemu_co_mutex_init(&s->lock);
> >      return 0;
> >  
> >   fail:
> > +    qemu_opts_del(opts);
> 
> You need to delete opts as well if this function succeeds, don't you?

New version will avoid the use of QemuOpts entirely - using the QDict
APIs instead, but I'll have equiv cleanup todo.

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