Re: [Qemu-devel] [PATCH v7 19/20] qcow2: report encryption specific image information

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, May 25, 2017 at 02:52:30PM -0500, Eric Blake wrote:
> On 05/25/2017 11:38 AM, Daniel P. Berrange wrote:
> > Currently 'qemu-img info' reports a simple "encrypted: yes"
> > field. This is not very useful now that qcow2 can support
> > multiple encryption formats. Users want to know which format
> > is in use and some data related to it.
> > 
> > Wire up usage of the qcrypto_block_get_info() method so that
> > 'qemu-img info' can report about the encryption format
> > and parameters in use
> > 
> 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  block/qcow2.c        | 35 ++++++++++++++++++++++++++++++++++-
> >  qapi/block-core.json | 24 +++++++++++++++++++++++-
> >  2 files changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 38f9eb5..cb321a2 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -3196,8 +3196,17 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> >  static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >  {
> >      BDRVQcow2State *s = bs->opaque;
> > -    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
> > +    ImageInfoSpecific *spec_info;
> > +    QCryptoBlockInfo *encrypt_info = NULL;
> >  
> > +    if (s->crypto != NULL) {
> > +        encrypt_info = qcrypto_block_get_info(s->crypto, NULL);
> 
> Worth using &error_abort instead of silently ignoring the error?  Is an
> error even possible in our output visitor [adding Markus for reference]?

In fact the qcrypto_block_get_info() will never return an error
right now as implemented. So I guess we could even just remove
the Error **errp parameter from that call

> 
> > +        if (!encrypt_info) {
> > +            return NULL;
> > +        }
> 
> If you use &error_abort, this is a dead check.
> 
> > +    }
> > +
> > +    spec_info = g_new(ImageInfoSpecific, 1);

> > +    if (encrypt_info) {
> > +        ImageInfoSpecificQCow2Encryption *qencrypt =
> > +            g_new(ImageInfoSpecificQCow2Encryption, 1);
> > +        switch (encrypt_info->format) {
> > +        case Q_CRYPTO_BLOCK_FORMAT_QCOW:
> > +            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_AES;
> > +            qencrypt->u.aes = encrypt_info->u.qcow;
> > +            break;
> > +        case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> > +            qencrypt->format = BLOCKDEV_QCOW2_ENCRYPTION_FORMAT_LUKS;
> > +            qencrypt->u.luks = encrypt_info->u.luks;
> > +            break;
> > +        default:
> > +            assert(false);
> > +        }
> > +        /* Since we did shallow copy above, erase any pointers
> > +         * in the original info */
> > +        memset(&encrypt_info->u, 0, sizeof(encrypt_info->u));
> > +        qapi_free_QCryptoBlockInfo(encrypt_info);
> 
> Does QAPI_CLONE_MEMBERS (commit 4626a19c) make this code any easier to
> write?  Then again, malloc'ing duplicates to then free the unchanged
> original is a bit slower than stealing references from the original then
> making sure the original can be freed without problem.

I don't think CLONE would make a signijficant difference to the size
of the code, since we have to remap the enum constants regardless.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|