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

[Max Reitz]

On 14.06.2016 17:47, Daniel P. Berrange wrote:
> On Tue, Jun 14, 2016 at 05:38:36PM +0200, Max Reitz wrote:
>> 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))?
> 
> I wish we could, but info->u.luks is an embedded QCryptoBlockInfoLUKS,
> not merely a pointer to an independently allocated QCryptoBlockInfoLUKS :-(

Yes, but note the asterisk I put there. Leave the g_new() and just make it:

spec_info->u.luks.data = g_new(QCryptoBlockInfoLUKS, 1);
*spec_info->u.luks.data = info->u.luks;

Max

signature.asc
Description: OpenPGP digital signature