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

[Daniel P. Berrange]

On Thu, May 11, 2017 at 04:05:59PM +0200, Alberto Garcia wrote:
> On Tue 25 Apr 2017 05:38:49 PM CEST, Daniel P. Berrange wrote:
> > @@ -181,8 +188,39 @@ static int qcow_open(BlockDriverState *bs, QDict 
> > *options, int flags,
>   [...]
> > +            crypto_opts = block_crypto_open_opts_init(
> > +                Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, &local_err);
> > +            if (local_err) {
> > +                error_propagate(errp, local_err);
> > +                ret = -EINVAL;
> > +                goto fail;
> > +            }
> 
> Not very important, but if you check !crypto_opts for errors instead you
> can pass errp directly and avoid that error_propagate() call. Exactly
> the same that you do here in qcrypto_block_open():
> 
> > +            if (flags & BDRV_O_NO_IO) {
> > +                cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> > +            }
> > +            s->crypto = qcrypto_block_open(crypto_opts, NULL, NULL,
> > +                                           cflags, errp);
> > +            if (!s->crypto) {
> > +                ret = -EINVAL;
> > +                goto fail;
> > +            }
> 
> 
> > @@ -792,6 +762,10 @@ static int qcow_create(const char *filename, QemuOpts 
> > *opts, Error **errp)
> >      int ret;
> >      BlockBackend *qcow_blk;
> >      const char *encryptfmt = NULL;
> > +    QDict *options;
> > +    QDict *encryptopts = NULL;
> > +    QCryptoBlockCreateOptions *crypto_opts = NULL;
> > +    QCryptoBlock *crypto = NULL;
> >  
> >      /* Read out options */
> >      total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> > @@ -865,6 +839,10 @@ static int qcow_create(const char *filename, QemuOpts 
> > *opts, Error **errp)
> >      l1_size = (total_size + (1LL << shift) - 1) >> shift;
> >  
> >      header.l1_table_offset = cpu_to_be64(header_size);
> > +
> > +    options = qemu_opts_to_qdict(opts, NULL);
> > +    qdict_extract_subqdict(options, &encryptopts, "encrypt.");
> > +    QDECREF(options);
> 
> I think you're leaking encryptopts in this function.
> 
> >  ##
> > +# @BlockdevQcowEncryptionFormat:
> > +# @qcow: AES-CBC with plain64 initialization venctors
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'enum': 'BlockdevQcowEncryptionFormat',
> > +  'data': [ 'qcow' ] }
> 
> Shouldn't this be 'aes' instead of 'qcow' ??

Opps, yes. I've been flipping back & forth about whether to call the
encryption format 'aes' or 'qcow' in the public API.  Internally
the crypto/ code calls it the 'qcow' encryption format, but from a
technical POV it is aes.

I see i've got the same mistake in the equivalent qcow2 patch too

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