Re: [Qemu-devel] [PATCH v5 13/18] qcow2: add support for LUKS encryption format

[Alberto Garcia]

On Tue 21 Feb 2017 12:55:07 PM CET, Daniel P. Berrange wrote:
>  static int qcow2_set_up_encryption(BlockDriverState *bs, QemuOpts *opts,
> -                                   Error **errp)
> +                                   const char *fmtstr, Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      QCryptoBlockCreateOptions *cryptoopts = NULL;
>      QCryptoBlock *crypto = NULL;
>      int ret = -EINVAL;
> +    int fmt = qcow2_crypt_method_from_format(fmtstr);
>  
> -    cryptoopts = block_crypto_create_opts_init(
> -        Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, "aes-", errp);
> +    switch (fmt) {
> +    case QCOW_CRYPT_LUKS:
> +        cryptoopts = block_crypto_create_opts_init(
> +            Q_CRYPTO_BLOCK_FORMAT_LUKS, opts, "luks-", errp);
> +        break;
> +    case QCOW_CRYPT_AES:
> +        cryptoopts = block_crypto_create_opts_init(
> +            Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, "aes-", errp);
> +        break;
> +    default:
> +        error_setg(errp, "Unknown encryption format %s", fmtstr);
> +        break;
> +    }
> +    s->crypt_method_header = fmt;
>      if (!cryptoopts) {
>          ret = -EINVAL;
>          goto out;
>      }

I don't believe it has any practical effect in this case, but I think
it's cleaner to set s->crypt_method_header = fmt after checking that
everything went well. Otherwise an incorrect format string will result
in s->crypt_method_header = -EINVAL, which doesn't make sense (it's not
even the correct type: the variable is unsigned).

Other than that, the patch looks good to me.

Reviewed-by: Alberto Garcia <address@hidden>

Berto