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

[Alberto Garcia]

On Thu 25 May 2017 06:38:40 PM CEST, Daniel P. Berrange wrote:
> @@ -105,6 +116,13 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      int ret;
>      QCowHeader header;
>      Error *local_err = NULL;
> +    QCryptoBlockOpenOptions *crypto_opts = NULL;
> +    unsigned int cflags = 0;
> +    QDict *encryptopts = NULL;
> +    const char *encryptfmt;
> +
> +    qdict_extract_subqdict(options, &encryptopts, "encrypt.");
> +    encryptfmt = qdict_get_try_str(encryptopts, "format");
>  
>      bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
>                                 false, errp);
>      if (!bs->file) {
>          return -EINVAL;
>      }

You're leaking encryptopts if the function returns here.

> @@ -873,6 +850,20 @@ static int qcow_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>              goto exit;
>          }
>          header.crypt_method = cpu_to_be32(QCOW_CRYPT_AES);
> +
> +        crypto_opts = block_crypto_create_opts_init(
> +            Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            ret = -EINVAL;
> +            goto exit;
> +        }

Not very important, and my fault for not having pointed it out in my
previous review, but you can spare the error_propagate() call if you
pass errp directly to block_crypto_create_opts_init() and then check if
crypto_opts is NULL.

Actually none of the error_propagate() calls in qcow_create() is really
necessary, but that could be fixed in a separate patch, if at all (it's
not so important).

The leak however needs to be fixed. With that,

Reviewed-by: Alberto Garcia <address@hidden>

Berto