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

[Daniel P. Berrange]

On Tue, Jun 14, 2016 at 05:49:24PM +0200, Max Reitz wrote:
> 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;

Oh whoops, I mis-read your original reply. I see what you mean now.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|