Re: [Qemu-block] [PATCH v2 2/2] block: export LUKS specific data to qemu-img info

[Max Reitz]

On 14.06.2016 12:56, Daniel P. Berrange wrote:
> The qemu-img info command has the ability to expose format
> specific metadata about volumes. Wire up this facility for
> the LUKS driver to report on cipher configuration and key
> slot usage.
> 
>     $ qemu-img info ~/VirtualMachines/demo.luks
>     image: /home/berrange/VirtualMachines/demo.luks
>     file format: luks
>     virtual size: 98M (102760448 bytes)
>     disk size: 100M
>     encrypted: yes
>     Format specific information:
>         ivgen alg: plain64
>         hash alg: sha1
>         cipher alg: aes-128
>         uuid: 6ddee74b-3a22-408c-8909-6789d4fa2594
>         cipher mode: xts
>         slots:
>             [0]:
>                 active: true
>                 iters: 572706
>                 key offset: 4096
>                 stripes: 4000
>             [1]:
>                 active: false
>                 key offset: 135168
>             [2]:
>                 active: false
>                 key offset: 266240
>             [3]:
>                 active: false
>                 key offset: 397312
>             [4]:
>                 active: false
>                 key offset: 528384
>             [5]:
>                 active: false
>                 key offset: 659456
>             [6]:
>                 active: false
>                 key offset: 790528
>             [7]:
>                 active: false
>                 key offset: 921600
>         payload offset: 2097152
>         master key iters: 142375
> 
> One somewhat undesirable artifact is that the data fields are
> printed out in (apparently) random order. This will be addressed
> later by changing the way the block layer pretty-prints the
> image specific data.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  block/crypto.c       | 59 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  7 ++++++-
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 758e14e..823572f 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -565,6 +565,63 @@ static int block_crypto_create_luks(const char *filename,
>                                         filename, opts, errp);
>  }
>  
> +static int block_crypto_get_info_luks(BlockDriverState *bs,
> +                                      BlockDriverInfo *bdi)
> +{
> +    BlockDriverInfo subbdi;
> +    int ret;
> +
> +    ret = bdrv_get_info(bs->file->bs, &subbdi);
> +    if (ret != 0) {
> +        return ret;
> +    }
> +
> +    bdi->unallocated_blocks_are_zero = false;
> +    bdi->can_write_zeroes_with_unmap = false;
> +    bdi->cluster_size = subbdi.cluster_size;
> +
> +    return 0;
> +}
> +
> +static ImageInfoSpecific *
> +block_crypto_get_specific_info_luks(BlockDriverState *bs)
> +{
> +    BlockCrypto *crypto = bs->opaque;
> +    ImageInfoSpecific *spec_info;
> +    QCryptoBlockInfo *info;
> +
> +    info = qcrypto_block_get_info(crypto->block, NULL);
> +    if (!info) {
> +        return NULL;
> +    }
> +    if (info->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) {
> +        qapi_free_QCryptoBlockInfo(info);
> +        return NULL;
> +    }
> +
> +    spec_info = g_new(ImageInfoSpecific, 1);
> +    spec_info->type =  IMAGE_INFO_SPECIFIC_KIND_LUKS;

One space too many.

> +    spec_info->u.luks.data = g_new(QCryptoBlockInfoLUKS, 1);
> +    spec_info->u.luks.data->cipher_alg = info->u.luks.cipher_alg;
> +    spec_info->u.luks.data->cipher_mode = info->u.luks.cipher_mode;
> +    spec_info->u.luks.data->ivgen_alg = info->u.luks.ivgen_alg;
> +    spec_info->u.luks.data->has_ivgen_hash_alg =
> +        info->u.luks.has_ivgen_hash_alg;
> +    spec_info->u.luks.data->ivgen_hash_alg = info->u.luks.ivgen_hash_alg;
> +    spec_info->u.luks.data->hash_alg = info->u.luks.hash_alg;
> +    spec_info->u.luks.data->payload_offset = info->u.luks.payload_offset;
> +    spec_info->u.luks.data->master_key_iters = info->u.luks.master_key_iters;
> +    spec_info->u.luks.data->uuid = g_strdup(info->u.luks.uuid);
> +
> +    /* Steal the pointer instead of bothering to copy */
> +    spec_info->u.luks.data->slots = info->u.luks.slots;
> +    info->u.luks.slots = NULL;

Why not just steal everything by *spec_info->u.luks.data = info->u.luks
and then making sure the qapi_free() call below won't free anything in
there with a memset(&info->u.luks, 0, sizeof(info->u.luks))?

> +
> +    qapi_free_QCryptoBlockInfo(info);
> +
> +    return spec_info;
> +}
> +
>  BlockDriver bdrv_crypto_luks = {
>      .format_name        = "luks",
>      .instance_size      = sizeof(BlockCrypto),
> @@ -578,6 +635,8 @@ BlockDriver bdrv_crypto_luks = {
>      .bdrv_co_readv      = block_crypto_co_readv,
>      .bdrv_co_writev     = block_crypto_co_writev,
>      .bdrv_getlength     = block_crypto_getlength,
> +    .bdrv_get_info      = block_crypto_get_info_luks,
> +    .bdrv_get_specific_info = block_crypto_get_specific_info_luks,
>  };
>  
>  static void block_crypto_init(void)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 98a20d2..959dd8b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -74,6 +74,7 @@
>        'extents': ['ImageInfo']
>    } }
>  
> +
>  ##
>  # @ImageInfoSpecific:
>  #

This hunk looks unnecessary.

Max

> @@ -85,7 +86,11 @@
>  { 'union': 'ImageInfoSpecific',
>    'data': {
>        'qcow2': 'ImageInfoSpecificQCow2',
> -      'vmdk': 'ImageInfoSpecificVmdk'
> +      'vmdk': 'ImageInfoSpecificVmdk',
> +      # If we need to add block driver specific parameters for
> +      # LUKS in future, then we'll subclass QCryptoBlockInfoLUKS
> +      # to define a ImageInfoSpecificLUKS
> +      'luks': 'QCryptoBlockInfoLUKS'
>    } }
>  
>  ##
>

signature.asc
Description: OpenPGP digital signature