| Message ID | 20170525163851.8047-20-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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]? > + if (!encrypt_info) { > + return NULL; > + } If you use &error_abort, this is a dead check. > + } > + > + spec_info = g_new(ImageInfoSpecific, 1); > *spec_info = (ImageInfoSpecific){ > .type = IMAGE_INFO_SPECIFIC_KIND_QCOW2, > .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1), > @@ -3224,6 +3233,30 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs) > assert(false); > } > > + 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. > # @ImageInfoSpecificQCow2: > # > # @compat: compatibility level > @@ -51,7 +72,8 @@ > 'compat': 'str', > '*lazy-refcounts': 'bool', > '*corrupt': 'bool', > - 'refcount-bits': 'int' > + 'refcount-bits': 'int', > + '*encrypt': 'ImageInfoSpecificQCow2Encryption' Missing documentation of the @encrypt field. Close!
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
"Daniel P. Berrange" <berrange@redhat.com> writes: > 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 Do you still need advice on QAPI from me here? [...]
On Mon, May 29, 2017 at 11:53:01AM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > 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 > > Do you still need advice on QAPI from me here? No need, thanks. Regards, Daniel
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); + if (!encrypt_info) { + return NULL; + } + } + + spec_info = g_new(ImageInfoSpecific, 1); *spec_info = (ImageInfoSpecific){ .type = IMAGE_INFO_SPECIFIC_KIND_QCOW2, .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1), @@ -3224,6 +3233,30 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs) assert(false); } + 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); + + spec_info->u.qcow2.data->has_encrypt = true; + spec_info->u.qcow2.data->encrypt = qencrypt; + } + return spec_info; } diff --git a/qapi/block-core.json b/qapi/block-core.json index a4e47a1..23b8f3f 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -33,6 +33,27 @@ 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } } ## +# @ImageInfoSpecificQCow2EncryptionBase: +# +# @format: The encryption format +# +# Since: 2.10 +## +{ 'struct': 'ImageInfoSpecificQCow2EncryptionBase', + 'data': { 'format': 'BlockdevQcow2EncryptionFormat'}} + +## +# @ImageInfoSpecificQCow2Encryption: +# +# Since: 2.10 +## +{ 'union': 'ImageInfoSpecificQCow2Encryption', + 'base': 'ImageInfoSpecificQCow2EncryptionBase', + 'discriminator': 'format', + 'data': { 'aes': 'QCryptoBlockInfoQCow', + 'luks': 'QCryptoBlockInfoLUKS' } } + +## # @ImageInfoSpecificQCow2: # # @compat: compatibility level @@ -51,7 +72,8 @@ 'compat': 'str', '*lazy-refcounts': 'bool', '*corrupt': 'bool', - 'refcount-bits': 'int' + 'refcount-bits': 'int', + '*encrypt': 'ImageInfoSpecificQCow2Encryption' } } ##
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 $ qemu-img create \ --object secret,id=sec0,data=123456 \ -o encrypt.format=luks,encrypt.key-secret=sec0 \ -f qcow2 demo.qcow2 1G Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \ encryption=off encrypt.format=luks encrypt.key-secret=sec0 \ cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ qemu-img info demo.qcow2 image: demo.qcow2 file format: qcow2 virtual size: 1.0G (1073741824 bytes) disk size: 480K encrypted: yes cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 encrypt: ivgen alg: plain64 hash alg: sha256 cipher alg: aes-256 uuid: 3fa930c4-58c8-4ef7-b3c5-314bb5af21f3 format: luks cipher mode: xts slots: [0]: active: true iters: 1839058 key offset: 4096 stripes: 4000 [1]: active: false key offset: 262144 [2]: active: false key offset: 520192 [3]: active: false key offset: 778240 [4]: active: false key offset: 1036288 [5]: active: false key offset: 1294336 [6]: active: false key offset: 1552384 [7]: active: false key offset: 1810432 payload offset: 2068480 master key iters: 438487 corrupt: false With the legacy "AES" encryption we just report the format name $ qemu-img create \ --object secret,id=sec0,data=123456 \ -o encrypt.format=aes,encrypt.key-secret=sec0 \ -f qcow2 demo.qcow2 1G Formatting 'demo.qcow2', fmt=qcow2 size=1073741824 \ encryption=off encrypt.format=aes encrypt.key-secret=sec0 \ cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ ./qemu-img info demo.qcow2 image: demo.qcow2 file format: qcow2 virtual size: 1.0G (1073741824 bytes) disk size: 196K encrypted: yes cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 encrypt: format: aes corrupt: false 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(-)