* [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver
@ 2016-06-07 10:11 Daniel P. Berrange
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption Daniel P. Berrange
` (7 more replies)
0 siblings, 8 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 10:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth, Eric Blake, Daniel P. Berrange
The 'qemu-img info' tool has ability to print format specific
information, eg with qcow2 it reports two extra items:
$ qemu-img info ~/VirtualMachines/demo.qcow2
image: /home/berrange/VirtualMachines/demo.qcow2
file format: qcow2
virtual size: 3.0G (3221225472 bytes)
disk size: 140K
cluster_size: 65536
Format specific information:
compat: 0.10
refcount bits: 16
This is not currently wired up for the LUKS driver. This patch
series adds that support so that we can report useful data about
the LUKS volume such as the crypto algorithm choices, key slot
usage and other volume metadata.
The first patch extends the crypto API to allow querying of the
format specific metadata
The second patches extends the block API to allow the LUKS driver
to report the format specific metadata.
$ 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:
cipher-alg: aes-128
cipher-mode: xts
ivgen-alg: plain64
hash-alg: sha1
payload-offset: 2097152
master-key-iters: 142375
uuid: 6ddee74b-3a22-408c-8909-6789d4fa2594
slots:
[0]:
active: true
iters: 572706
stripes: 4000
key-offset: 8
[1]:
active: false
iters: 0
stripes: 4000
key-offset: 264
[2]:
active: false
iters: 0
stripes: 4000
key-offset: 520
[3]:
active: false
iters: 0
stripes: 4000
key-offset: 776
[4]:
active: false
iters: 0
stripes: 4000
key-offset: 1032
[5]:
active: false
iters: 0
stripes: 4000
key-offset: 1288
[6]:
active: false
iters: 0
stripes: 4000
key-offset: 1544
[7]:
active: false
iters: 0
stripes: 4000
key-offset: 1800
The remaining 4 patches are improvements to QAPI and the core
block layer to fix a problem whereby struct fields are output
in (apparently) random ordering. This is because the QAPI type
is converted into a QObject for pretty-printing, thus throwing
away any struct field ordering information.
To address this I created a new TextOutputVisitor which can
directly pretty-print QAPI types. I then changed the code
generator to create qapi_stringify_TYPENAME() methods for
all QAPI types. Finally I changed the block layer over to
use this stringify approach instead.
I'm not sure if QAPI maintainers will find the idea of adding
qapi_stringify_TYPENAME() methods desirable or not. It is of
course valid to directly use the TextOutputVisitor from the
block layer. I felt there might be some use in debugging to
have a convenient qapi_stringify_TYPENAME() method around
though - personally I often find it helpful to be able to
easily dump an QAPI object or any QObject to a humand friendly
format for debugging and the less code I need write to add
this temporary debug output the better.
Daniel P. Berrange (6):
crypto: add support for querying parameters for block encryption
block: export LUKS specific data to qemu-img info
qapi: assert that visitor impls have required callbacks
qapi: add a text output visitor for pretty printing types
qapi: generate a qapi_stringify_TYPENAME method for all types
block: convert to use qapi_stringify_ImageInfoSpecific
block/crypto.c | 80 +++++++++++++
block/qapi.c | 101 +---------------
crypto/block-luks.c | 66 ++++++++++-
crypto/block.c | 17 +++
crypto/blockpriv.h | 4 +
include/crypto/block.h | 16 +++
include/qapi/text-output-visitor.h | 73 ++++++++++++
include/qapi/visitor-impl.h | 5 +-
include/qapi/visitor.h | 5 +-
qapi/Makefile.objs | 1 +
qapi/block-core.json | 34 +++++-
qapi/crypto.json | 65 ++++++++++
qapi/opts-visitor.c | 5 +-
qapi/qapi-dealloc-visitor.c | 4 +-
qapi/qapi-visit-core.c | 24 +++-
qapi/qmp-input-visitor.c | 5 +-
qapi/qmp-output-visitor.c | 4 +-
qapi/string-input-visitor.c | 5 +-
qapi/string-output-visitor.c | 5 +-
qapi/text-output-visitor.c | 235 +++++++++++++++++++++++++++++++++++++
scripts/qapi-types.py | 45 +++++++
scripts/qapi-visit.py | 5 +-
22 files changed, 682 insertions(+), 122 deletions(-)
create mode 100644 include/qapi/text-output-visitor.h
create mode 100644 qapi/text-output-visitor.c
--
2.5.5
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
@ 2016-06-07 10:11 ` Daniel P. Berrange
2016-06-07 14:17 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info Daniel P. Berrange
` (6 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 10:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth, Eric Blake, Daniel P. Berrange
When creating new block encryption volumes, we accept a list of
parameters to control the formatting process. It is useful to
be able to query what those parameters were for existing block
devices. Add a qcrypto_block_get_info() method which returns a
QCryptoBlockInfo instance to report this data.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
crypto/block-luks.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++-
crypto/block.c | 17 +++++++++++++
crypto/blockpriv.h | 4 +++
include/crypto/block.h | 16 ++++++++++++
qapi/crypto.json | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 167 insertions(+), 1 deletion(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 17c4300..1c8e4d6 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -201,6 +201,15 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592);
struct QCryptoBlockLUKS {
QCryptoBlockLUKSHeader header;
+
+ /* Cache parsed versions of what's in header fields.
+ * as we can't rely on QCryptoBlock.cipher being
+ * non-NULL */
+ QCryptoCipherAlgorithm cipher_alg;
+ QCryptoCipherMode cipher_mode;
+ QCryptoIVGenAlgorithm ivgen_alg;
+ QCryptoHashAlgorithm ivgen_hash_alg;
+ QCryptoHashAlgorithm hash_alg;
};
@@ -835,6 +844,12 @@ qcrypto_block_luks_open(QCryptoBlock *block,
block->payload_offset = luks->header.payload_offset *
QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
+ luks->cipher_alg = cipheralg;
+ luks->cipher_mode = ciphermode;
+ luks->ivgen_alg = ivalg;
+ luks->ivgen_hash_alg = ivhash;
+ luks->hash_alg = hash;
+
g_free(masterkey);
g_free(password);
@@ -947,7 +962,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
}
hash_alg = QCryptoHashAlgorithm_lookup[luks_opts.hash_alg];
-
if (strlen(cipher_alg) >= QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN) {
error_setg(errp, "Cipher name '%s' is too long for LUKS header",
cipher_alg);
@@ -1252,6 +1266,12 @@ qcrypto_block_luks_create(QCryptoBlock *block,
goto error;
}
+ luks->cipher_alg = luks_opts.cipher_alg;
+ luks->cipher_mode = luks_opts.cipher_mode;
+ luks->ivgen_alg = luks_opts.ivgen_alg;
+ luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg;
+ luks->hash_alg = luks_opts.hash_alg;
+
memset(masterkey, 0, luks->header.key_bytes);
g_free(masterkey);
memset(slotkey, 0, luks->header.key_bytes);
@@ -1286,6 +1306,49 @@ qcrypto_block_luks_create(QCryptoBlock *block,
}
+static int qcrypto_block_luks_get_info(QCryptoBlock *block,
+ QCryptoBlockInfo *info,
+ Error **errp)
+{
+ QCryptoBlockLUKS *luks = block->opaque;
+ QCryptoBlockInfoLUKSSlot *slot;
+ QCryptoBlockInfoLUKSSlotList *slots = NULL, *prev = NULL;
+ size_t i;
+
+ info->u.luks.cipher_alg = luks->cipher_alg;
+ info->u.luks.cipher_mode = luks->cipher_mode;
+ info->u.luks.ivgen_alg = luks->ivgen_alg;
+ if (info->u.luks.ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) {
+ info->u.luks.has_ivgen_hash_alg = true;
+ info->u.luks.ivgen_hash_alg = luks->ivgen_hash_alg;
+ }
+ info->u.luks.hash_alg = luks->hash_alg;
+ info->u.luks.payload_offset = block->payload_offset;
+ info->u.luks.master_key_iters = luks->header.master_key_iterations;
+ info->u.luks.uuid = g_strdup((const char *)luks->header.uuid);
+
+ for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
+ slots = g_new0(QCryptoBlockInfoLUKSSlotList, 1);
+ if (i == 0) {
+ info->u.luks.slots = slots;
+ } else {
+ prev->next = slots;
+ }
+
+ slots->value = slot = g_new0(QCryptoBlockInfoLUKSSlot, 1);
+ slot->active = luks->header.key_slots[i].active ==
+ QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
+ slot->iters = luks->header.key_slots[i].iterations;
+ slot->stripes = luks->header.key_slots[i].stripes;
+ slot->key_offset = luks->header.key_slots[i].key_offset;
+
+ prev = slots;
+ }
+
+ return 0;
+}
+
+
static void qcrypto_block_luks_cleanup(QCryptoBlock *block)
{
g_free(block->opaque);
@@ -1323,6 +1386,7 @@ qcrypto_block_luks_encrypt(QCryptoBlock *block,
const QCryptoBlockDriver qcrypto_block_driver_luks = {
.open = qcrypto_block_luks_open,
.create = qcrypto_block_luks_create,
+ .get_info = qcrypto_block_luks_get_info,
.cleanup = qcrypto_block_luks_cleanup,
.decrypt = qcrypto_block_luks_decrypt,
.encrypt = qcrypto_block_luks_encrypt,
diff --git a/crypto/block.c b/crypto/block.c
index da60eba..be823ee 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -105,6 +105,23 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
}
+QCryptoBlockInfo *qcrypto_block_get_info(QCryptoBlock *block,
+ Error **errp)
+{
+ QCryptoBlockInfo *info = g_new0(QCryptoBlockInfo, 1);
+
+ info->format = block->format;
+
+ if (block->driver->get_info &&
+ block->driver->get_info(block, info, errp) < 0) {
+ g_free(info);
+ return NULL;
+ }
+
+ return info;
+}
+
+
int qcrypto_block_decrypt(QCryptoBlock *block,
uint64_t startsector,
uint8_t *buf,
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 6297085..35217cd 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -53,6 +53,10 @@ struct QCryptoBlockDriver {
void *opaque,
Error **errp);
+ int (*get_info)(QCryptoBlock *block,
+ QCryptoBlockInfo *info,
+ Error **errp);
+
void (*cleanup)(QCryptoBlock *block);
int (*encrypt)(QCryptoBlock *block,
diff --git a/include/crypto/block.h b/include/crypto/block.h
index a21e11f..5ce18d9 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -138,6 +138,22 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
void *opaque,
Error **errp);
+
+/**
+ * qcrypto_block_get_info:
+ * block:L the block encryption object
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Get information about the configuration options for the
+ * block encryption object. This includes details such as
+ * the cipher algorithms, modes, and initialization vector
+ * generators.
+ *
+ * Returns: a block encryption info object, or NULL on error
+ */
+QCryptoBlockInfo *qcrypto_block_get_info(QCryptoBlock *block,
+ Error **errp);
+
/**
* @qcrypto_block_decrypt:
* @block: the block encryption object
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 760d0c0..29bfeb2 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -220,3 +220,68 @@
'discriminator': 'format',
'data': { 'qcow': 'QCryptoBlockOptionsQCow',
'luks': 'QCryptoBlockCreateOptionsLUKS' } }
+
+
+##
+# QCryptoBlockInfoBase:
+#
+# The common information that applies to all full disk
+# encryption formats
+#
+# @format: the encryption format
+#
+# Since: 2.7
+##
+{ 'struct': 'QCryptoBlockInfoBase',
+ 'data': { 'format': 'QCryptoBlockFormat' }}
+
+
+##
+# QCryptoBlockInfoLUKSSlot:
+#
+# Information about the LUKS block encryption key
+# slot options
+#
+{ 'struct': 'QCryptoBlockInfoLUKSSlot',
+ 'data': {'active': 'bool',
+ 'iters': 'int',
+ 'stripes': 'int',
+ 'key-offset': 'int' } }
+
+
+##
+# QCryptoBlockInfoLUKS:
+#
+# Information about the LUKS block encryption options
+#
+# @cipher-alg: the cipher algorithm for data encryption
+# @cipher-mode: the cipher mode for data encryption
+# @ivgen-alg: the initialization vector generator
+# @ivgen-hash-alg: the initialization vector generator hash
+# @hash-alg: the master key hash algorithm
+#
+# Since: 2.7
+##
+{ 'struct': 'QCryptoBlockInfoLUKS',
+ 'data': {'cipher-alg': 'QCryptoCipherAlgorithm',
+ 'cipher-mode': 'QCryptoCipherMode',
+ 'ivgen-alg': 'QCryptoIVGenAlgorithm',
+ '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
+ 'hash-alg': 'QCryptoHashAlgorithm',
+ 'payload-offset': 'int',
+ 'master-key-iters': 'int',
+ 'uuid': 'str',
+ 'slots': [ 'QCryptoBlockInfoLUKSSlot' ] }}
+
+
+##
+# QCryptoBlockInfo:
+#
+# Information about the block encryption options
+#
+# Since: 2.7
+##
+{ 'union': 'QCryptoBlockInfo',
+ 'base': 'QCryptoBlockInfoBase',
+ 'discriminator': 'format',
+ 'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
--
2.5.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption Daniel P. Berrange
@ 2016-06-07 10:11 ` Daniel P. Berrange
2016-06-07 15:36 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks Daniel P. Berrange
` (5 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 10:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth, Eric Blake, Daniel P. Berrange
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: 8
stripes: 4000
[1]:
active: false
iters: 0
key offset: 264
stripes: 4000
[2]:
active: false
iters: 0
key offset: 520
stripes: 4000
[3]:
active: false
iters: 0
key offset: 776
stripes: 4000
[4]:
active: false
iters: 0
key offset: 1032
stripes: 4000
[5]:
active: false
iters: 0
key offset: 1288
stripes: 4000
[6]:
active: false
iters: 0
key offset: 1544
stripes: 4000
[7]:
active: false
iters: 0
key offset: 1800
stripes: 4000
payload offset: 2097152
master key iters: 142375
One somewhat undesirable artifact is that the data fields are
printed out in (apparantly) 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 <berrange@redhat.com>
---
block/crypto.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
qapi/block-core.json | 34 +++++++++++++++++++++-
2 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/block/crypto.c b/block/crypto.c
index 758e14e..6f12c4d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -565,6 +565,84 @@ 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;
+ QCryptoBlockInfoLUKSSlot *luks_slot;
+ QCryptoBlockInfoLUKSSlotList *luks_slots;
+ ImageInfoSpecificLUKSSlot *slot;
+ ImageInfoSpecificLUKSSlotList *slots, *prev = NULL;
+
+ 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;
+ spec_info->u.luks.data = g_new(ImageInfoSpecificLUKS, 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);
+
+ luks_slots = info->u.luks.slots;
+ while (luks_slots) {
+ luks_slot = luks_slots->value;
+
+ slots = g_new0(ImageInfoSpecificLUKSSlotList, 1);
+ if (prev == NULL) {
+ spec_info->u.luks.data->slots = slots;
+ } else {
+ prev->next = slots;
+ }
+
+ slots->value = slot = g_new0(ImageInfoSpecificLUKSSlot, 1);
+ slot->active = luks_slot->active;
+ slot->iters = luks_slot->iters;
+ slot->stripes = luks_slot->stripes;
+ slot->key_offset = luks_slot->key_offset;
+
+ prev = slots;
+ luks_slots = luks_slots->next;
+ }
+
+ qapi_free_QCryptoBlockInfo(info);
+
+ return spec_info;
+}
+
BlockDriver bdrv_crypto_luks = {
.format_name = "luks",
.instance_size = sizeof(BlockCrypto),
@@ -578,6 +656,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..58a6093 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -74,6 +74,37 @@
'extents': ['ImageInfo']
} }
+
+{ 'struct': 'ImageInfoSpecificLUKSSlot',
+ 'data': {'active': 'bool',
+ 'iters': 'int',
+ 'stripes': 'int',
+ 'key-offset': 'int' } }
+
+##
+# @ImageInfoSpecificLUKS:
+#
+# @cipher-alg: the cipher algorithm for data encryption
+# @cipher-mode: the cipher mode for data encryption
+# @ivgen-alg: the initialization vector generator
+# @ivgen-hash-alg: the initialization vector generator hash
+# @hash-alg: the master key hash algorithm
+#
+# Since: 2.7
+##
+{ 'struct': 'ImageInfoSpecificLUKS',
+ 'data': {
+ 'cipher-alg': 'QCryptoCipherAlgorithm',
+ 'cipher-mode': 'QCryptoCipherMode',
+ 'ivgen-alg': 'QCryptoIVGenAlgorithm',
+ '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
+ 'hash-alg': 'QCryptoHashAlgorithm',
+ 'payload-offset': 'int',
+ 'master-key-iters': 'int',
+ 'uuid': 'str',
+ 'slots': [ 'ImageInfoSpecificLUKSSlot' ]
+ } }
+
##
# @ImageInfoSpecific:
#
@@ -85,7 +116,8 @@
{ 'union': 'ImageInfoSpecific',
'data': {
'qcow2': 'ImageInfoSpecificQCow2',
- 'vmdk': 'ImageInfoSpecificVmdk'
+ 'vmdk': 'ImageInfoSpecificVmdk',
+ 'luks': 'ImageInfoSpecificLUKS'
} }
##
--
2.5.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption Daniel P. Berrange
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info Daniel P. Berrange
@ 2016-06-07 10:11 ` Daniel P. Berrange
2016-06-07 15:40 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types Daniel P. Berrange
` (4 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 10:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth, Eric Blake, Daniel P. Berrange
Not all visitor implementations supply the full set of
visitor callback functions. For example, the string
output visitor does not provide 'start_struct' and
friends. If you don't know this and feed it an object
that uses structs, you'll get a crash:
Segmentation fault (core dumped)
Crashing is fine, because this is a programmer mistake,
but we can improve the error message upon crash to make
it obvious what failed by adding assert()s:
qapi/qapi-visit-core.c:32: visit_start_struct: Assertion `v->start_struct != ((void *)0)' failed.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qapi/qapi-visit-core.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index eada467..3b5efbe 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -29,6 +29,7 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
assert(size);
assert(v->type != VISITOR_OUTPUT || *obj);
}
+ assert(v->start_struct != NULL);
v->start_struct(v, name, obj, size, &err);
if (obj && v->type == VISITOR_INPUT) {
assert(!err != !*obj);
@@ -45,6 +46,7 @@ void visit_check_struct(Visitor *v, Error **errp)
void visit_end_struct(Visitor *v)
{
+ assert(v->end_struct != NULL);
v->end_struct(v);
}
@@ -54,6 +56,7 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list,
Error *err = NULL;
assert(!list || size >= sizeof(GenericList));
+ assert(v->start_list != NULL);
v->start_list(v, name, list, size, &err);
if (list && v->type == VISITOR_INPUT) {
assert(!(err && *list));
@@ -64,11 +67,13 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list,
GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
{
assert(tail && size >= sizeof(GenericList));
+ assert(v->next_list != NULL);
return v->next_list(v, tail, size);
}
void visit_end_list(Visitor *v)
{
+ assert(v->end_list != NULL);
v->end_list(v);
}
@@ -112,6 +117,7 @@ bool visit_is_input(Visitor *v)
void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
{
assert(obj);
+ assert(v->type_int64 != NULL);
v->type_int64(v, name, obj, errp);
}
@@ -121,6 +127,7 @@ static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
Error *err = NULL;
uint64_t value = *obj;
+ assert(v->type_uint64 != NULL);
v->type_uint64(v, name, &value, &err);
if (err) {
error_propagate(errp, err);
@@ -160,6 +167,7 @@ void visit_type_uint64(Visitor *v, const char *name, uint64_t *obj,
Error **errp)
{
assert(obj);
+ assert(v->type_uint64 != NULL);
v->type_uint64(v, name, obj, errp);
}
@@ -170,6 +178,7 @@ static void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
Error *err = NULL;
int64_t value = *obj;
+ assert(v->type_int64 != NULL);
v->type_int64(v, name, &value, &err);
if (err) {
error_propagate(errp, err);
@@ -218,6 +227,7 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
if (v->type_size) {
v->type_size(v, name, obj, errp);
} else {
+ assert(v->type_uint64 != NULL);
v->type_uint64(v, name, obj, errp);
}
}
@@ -225,6 +235,7 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
{
assert(obj);
+ assert(v->type_bool != NULL);
v->type_bool(v, name, obj, errp);
}
@@ -237,6 +248,7 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
* can enable:
assert(v->type != VISITOR_OUTPUT || *obj);
*/
+ assert(v->type_str != NULL);
v->type_str(v, name, obj, &err);
if (v->type == VISITOR_INPUT) {
assert(!err != !*obj);
@@ -248,6 +260,7 @@ void visit_type_number(Visitor *v, const char *name, double *obj,
Error **errp)
{
assert(obj);
+ assert(v->type_number != NULL);
v->type_number(v, name, obj, errp);
}
@@ -257,6 +270,7 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
assert(obj);
assert(v->type != VISITOR_OUTPUT || *obj);
+ assert(v->type_any != NULL);
v->type_any(v, name, obj, &err);
if (v->type == VISITOR_INPUT) {
assert(!err != !*obj);
@@ -266,6 +280,7 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
void visit_type_null(Visitor *v, const char *name, Error **errp)
{
+ assert(v->type_null != NULL);
v->type_null(v, name, errp);
}
--
2.5.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
` (2 preceding siblings ...)
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks Daniel P. Berrange
@ 2016-06-07 10:11 ` Daniel P. Berrange
2016-06-07 16:09 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 5/6] qapi: generate a qapi_stringify_TYPENAME method for all types Daniel P. Berrange
` (3 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 10:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth, Eric Blake, Daniel P. Berrange
The current approach for pretty-printing QAPI types is to
convert them to JSON using the QMP output visitor and then
pretty-print the JSON document. This has an unfixable problem
that structs get their keys printed out in random order, since
JSON dicts do not contain any key ordering information.
To address this, introduce a text output visitor that can
directly pretty print a QAPI type into a string.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/qapi/text-output-visitor.h | 73 ++++++++++++
include/qapi/visitor-impl.h | 5 +-
include/qapi/visitor.h | 5 +-
qapi/Makefile.objs | 1 +
qapi/opts-visitor.c | 5 +-
qapi/qapi-dealloc-visitor.c | 4 +-
qapi/qapi-visit-core.c | 9 +-
qapi/qmp-input-visitor.c | 5 +-
qapi/qmp-output-visitor.c | 4 +-
qapi/string-input-visitor.c | 5 +-
qapi/string-output-visitor.c | 5 +-
qapi/text-output-visitor.c | 235 +++++++++++++++++++++++++++++++++++++
scripts/qapi-visit.py | 5 +-
13 files changed, 339 insertions(+), 22 deletions(-)
create mode 100644 include/qapi/text-output-visitor.h
create mode 100644 qapi/text-output-visitor.c
diff --git a/include/qapi/text-output-visitor.h b/include/qapi/text-output-visitor.h
new file mode 100644
index 0000000..3b742b7
--- /dev/null
+++ b/include/qapi/text-output-visitor.h
@@ -0,0 +1,73 @@
+/*
+ * Text pretty printing Visitor
+ *
+ * Copyright Red Hat, Inc. 2016
+ *
+ * Author: Daniel Berrange <berrange@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef TEXT_OUTPUT_VISITOR_H
+#define TEXT_OUTPUT_VISITOR_H
+
+#include "qapi/visitor.h"
+
+typedef struct TextOutputVisitor TextOutputVisitor;
+
+/**
+ * text_output_visitor_new:
+ * @extraIndent: number of extra levels of indent to apply
+ * @skipLevel: skip output of nodes less than depth @skipLevel
+ *
+ * Create a new output visitor for displaying objects
+ * in a pretty-printed text format. The @extraIdent
+ * parameter can be used to add extra levels of whitespace
+ * indentation on the output text. If there are some nodes
+ * at the top level of the QAPI object that should not be
+ * displayed, the @skipLevel parameter can be set to a
+ * non-zero value to hide them
+ *
+ * Returns: a pointer to new output visitor
+ */
+TextOutputVisitor *text_output_visitor_new(int extraIndent,
+ int skipLevel);
+
+/**
+ * text_output_visitor_cleanup:
+ * @v: the output visitor object
+ *
+ * Release all resources associated with the output
+ * visitor
+ */
+void text_output_visitor_cleanup(TextOutputVisitor *v);
+
+/**
+ * text_output_get_string:
+ * @v: the output visitor object
+ *
+ * Get the string containing the pretty-printed
+ * text output of the object(s) that have been
+ * visited. The memory for the returned string
+ * should be released with g_free() when no longer
+ * required.
+ *
+ * Returns: a string containing output.
+ */
+char *text_output_get_string(TextOutputVisitor *v);
+
+/**
+ * text_output_get_visitor
+ * @v: the output visitor object
+ *
+ * Get the visitor instance to be used to visit
+ * QAPI objects. The returned instance should not
+ * be freed by the callers.
+ *
+ * Returns: a visitor instance
+ */
+Visitor *text_output_get_visitor(TextOutputVisitor *v);
+
+#endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 145afd0..23cc6aa 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -52,10 +52,11 @@ struct Visitor
/* Must be set; implementations may require @list to be non-null,
* but must document it. */
void (*start_list)(Visitor *v, const char *name, GenericList **list,
- size_t size, Error **errp);
+ size_t size, bool hasElements, Error **errp);
/* Must be set */
- GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
+ GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size,
+ size_t element);
/* Must be set */
void (*end_list)(Visitor *v);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 4d12167..c8c39d7 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -303,7 +303,7 @@ void visit_end_struct(Visitor *v);
* even if intermediate visits fail. See the examples above.
*/
void visit_start_list(Visitor *v, const char *name, GenericList **list,
- size_t size, Error **errp);
+ size_t size, bool hasElements, Error **errp);
/*
* Iterate over a GenericList during a non-virtual list visit.
@@ -319,7 +319,8 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list,
* the list, with that function's name parameter set to NULL and obj
* set to the address of @tail->value.
*/
-GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
+GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size,
+ size_t element);
/*
* Complete a list visit started earlier.
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 2278970..7caf64b 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -1,6 +1,7 @@
util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
util-obj-y += string-input-visitor.o string-output-visitor.o
+util-obj-y += text-output-visitor.o
util-obj-y += opts-visitor.o
util-obj-y += qmp-event.o
util-obj-y += qapi-util.o
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 4cf1cf8..8c59597 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -214,7 +214,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
static void
opts_start_list(Visitor *v, const char *name, GenericList **list, size_t size,
- Error **errp)
+ bool hasElements, Error **errp)
{
OptsVisitor *ov = to_ov(v);
@@ -233,7 +233,8 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, size_t size,
static GenericList *
-opts_next_list(Visitor *v, GenericList *tail, size_t size)
+opts_next_list(Visitor *v, GenericList *tail,
+ size_t size, size_t element)
{
OptsVisitor *ov = to_ov(v);
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index cd68b55..025b7d3 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -90,12 +90,12 @@ static void qapi_dealloc_end_alternate(Visitor *v)
static void qapi_dealloc_start_list(Visitor *v, const char *name,
GenericList **list, size_t size,
- Error **errp)
+ bool hasElements, Error **errp)
{
}
static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *tail,
- size_t size)
+ size_t size, size_t element)
{
GenericList *next = tail->next;
g_free(tail);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 3b5efbe..a2f31d5 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -51,24 +51,25 @@ void visit_end_struct(Visitor *v)
}
void visit_start_list(Visitor *v, const char *name, GenericList **list,
- size_t size, Error **errp)
+ size_t size, bool hasElements, Error **errp)
{
Error *err = NULL;
assert(!list || size >= sizeof(GenericList));
assert(v->start_list != NULL);
- v->start_list(v, name, list, size, &err);
+ v->start_list(v, name, list, size, hasElements, &err);
if (list && v->type == VISITOR_INPUT) {
assert(!(err && *list));
}
error_propagate(errp, err);
}
-GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
+GenericList *visit_next_list(Visitor *v, GenericList *tail,
+ size_t size, size_t element)
{
assert(tail && size >= sizeof(GenericList));
assert(v->next_list != NULL);
- return v->next_list(v, tail, size);
+ return v->next_list(v, tail, size, element);
}
void visit_end_list(Visitor *v)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index aea90a1..2509af4 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -192,7 +192,8 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
static void qmp_input_start_list(Visitor *v, const char *name,
- GenericList **list, size_t size, Error **errp)
+ GenericList **list, size_t size,
+ bool hasElements, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
QObject *qobj = qmp_input_get_object(qiv, name, true);
@@ -218,7 +219,7 @@ static void qmp_input_start_list(Visitor *v, const char *name,
}
static GenericList *qmp_input_next_list(Visitor *v, GenericList *tail,
- size_t size)
+ size_t size, size_t element)
{
QmpInputVisitor *qiv = to_qiv(v);
StackObject *so = &qiv->stack[qiv->nb_stack - 1];
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 4d3cf78..395a8a3 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -116,7 +116,7 @@ static void qmp_output_end_struct(Visitor *v)
static void qmp_output_start_list(Visitor *v, const char *name,
GenericList **listp, size_t size,
- Error **errp)
+ bool hasElements, Error **errp)
{
QmpOutputVisitor *qov = to_qov(v);
QList *list = qlist_new();
@@ -126,7 +126,7 @@ static void qmp_output_start_list(Visitor *v, const char *name,
}
static GenericList *qmp_output_next_list(Visitor *v, GenericList *tail,
- size_t size)
+ size_t size, size_t element)
{
return tail->next;
}
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 30b5879..ab804c8 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -127,7 +127,7 @@ error:
static void
start_list(Visitor *v, const char *name, GenericList **list, size_t size,
- Error **errp)
+ bool hasElements, Error **errp)
{
StringInputVisitor *siv = to_siv(v);
@@ -151,7 +151,8 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size,
}
}
-static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
+static GenericList *next_list(Visitor *v, GenericList *tail,
+ size_t size, size_t element)
{
StringInputVisitor *siv = to_siv(v);
Range *r;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index d013196..a645a47 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -266,7 +266,7 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
static void
start_list(Visitor *v, const char *name, GenericList **list, size_t size,
- Error **errp)
+ bool hasElements, Error **errp)
{
StringOutputVisitor *sov = to_sov(v);
@@ -280,7 +280,8 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size,
}
}
-static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
+static GenericList *next_list(Visitor *v, GenericList *tail,
+ size_t size, size_t element)
{
StringOutputVisitor *sov = to_sov(v);
GenericList *ret = tail->next;
diff --git a/qapi/text-output-visitor.c b/qapi/text-output-visitor.c
new file mode 100644
index 0000000..35afe58
--- /dev/null
+++ b/qapi/text-output-visitor.c
@@ -0,0 +1,235 @@
+/*
+ * Text pretty printing Visitor
+ *
+ * Copyright Red Hat, Inc. 2016
+ *
+ * Author: Daniel Berrange <berrange@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/text-output-visitor.h"
+#include "qapi/visitor-impl.h"
+#include <math.h>
+
+struct TextOutputVisitor {
+ Visitor visitor;
+ GString *string;
+ int level;
+ int skipLevel;
+ int extraIndent;
+};
+
+static TextOutputVisitor *to_sov(Visitor *v)
+{
+ return container_of(v, TextOutputVisitor, visitor);
+}
+
+
+#define INDENT (sov->extraIndent + ((sov->level - sov->skipLevel) * 4))
+
+static void print_type_int64(Visitor *v, const char *name, int64_t *obj,
+ Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+
+ if (sov->level < sov->skipLevel) {
+ return;
+ }
+ g_string_append_printf(sov->string,
+ "%*s%s: %" PRIu64 "\n",
+ INDENT, "",
+ name, *obj);
+}
+
+static void print_type_uint64(Visitor *v, const char *name, uint64_t *obj,
+ Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+
+ if (sov->level < sov->skipLevel) {
+ return;
+ }
+ g_string_append_printf(sov->string,
+ "%*s%s: %" PRIi64 "\n",
+ INDENT, "",
+ name, *obj);
+}
+
+static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
+ Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+ static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
+ uint64_t div, val;
+ int i;
+
+ if (sov->level < sov->skipLevel) {
+ return;
+ }
+
+ val = *obj;
+
+ /* The exponent (returned in i) minus one gives us
+ * floor(log2(val * 1024 / 1000). The correction makes us
+ * switch to the higher power when the integer part is >= 1000.
+ */
+ frexp(val / (1000.0 / 1024.0), &i);
+ i = (i - 1) / 10;
+ assert(i < ARRAY_SIZE(suffixes));
+ div = 1ULL << (i * 10);
+
+ g_string_append_printf(sov->string,
+ "%*s%s: %" PRIu64 " (%0.3g %c%s)\n",
+ INDENT, "",
+ name, val,
+ (double)val / div, suffixes[i], i ? "iB" : "");
+}
+
+static void print_type_bool(Visitor *v, const char *name, bool *obj,
+ Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+
+ if (sov->level < sov->skipLevel) {
+ return;
+ }
+ g_string_append_printf(sov->string,
+ "%*s%s: %s\n",
+ INDENT, "",
+ name, *obj ? "true" : "false");
+}
+
+static void print_type_str(Visitor *v, const char *name, char **obj,
+ Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+
+ if (sov->level < sov->skipLevel) {
+ return;
+ }
+ g_string_append_printf(sov->string,
+ "%*s%s: %s\n",
+ INDENT, "",
+ name, *obj ? *obj : "<null>");
+}
+
+static void print_type_number(Visitor *v, const char *name, double *obj,
+ Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+
+ if (sov->level < sov->skipLevel) {
+ return;
+ }
+ g_string_append_printf(sov->string,
+ "%*s%s: %f\n",
+ INDENT, "",
+ name, *obj);
+}
+
+static void
+start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+ bool hasElements, Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+
+ if (sov->level >= sov->skipLevel) {
+ g_string_append_printf(sov->string,
+ "%*s%s:\n",
+ INDENT, "",
+ name);
+ }
+ sov->level++;
+ if (hasElements) {
+ if (sov->level >= sov->skipLevel) {
+ g_string_append_printf(sov->string,
+ "%*s[0]:\n",
+ INDENT, "");
+ }
+ }
+}
+
+static GenericList *next_list(Visitor *v, GenericList *tail,
+ size_t size, size_t element)
+{
+ TextOutputVisitor *sov = to_sov(v);
+ GenericList *ret = tail->next;
+ if (ret && sov->level >= sov->skipLevel) {
+ g_string_append_printf(sov->string,
+ "%*s[%zu]:\n",
+ INDENT, "", element);
+ }
+ return ret;
+}
+
+static void end_list(Visitor *v)
+{
+ TextOutputVisitor *sov = to_sov(v);
+ sov->level--;
+}
+
+static void start_struct(Visitor *v, const char *name, void **obj,
+ size_t size, Error **errp)
+{
+ TextOutputVisitor *sov = to_sov(v);
+ sov->level++;
+}
+
+
+static void end_struct(Visitor *v)
+{
+ TextOutputVisitor *sov = to_sov(v);
+ sov->level--;
+}
+
+
+char *text_output_get_string(TextOutputVisitor *sov)
+{
+ char *string = g_string_free(sov->string, false);
+ sov->string = NULL;
+ return string;
+}
+
+Visitor *text_output_get_visitor(TextOutputVisitor *sov)
+{
+ return &sov->visitor;
+}
+
+void text_output_visitor_cleanup(TextOutputVisitor *sov)
+{
+ if (sov->string) {
+ g_string_free(sov->string, true);
+ }
+ g_free(sov);
+}
+
+TextOutputVisitor *text_output_visitor_new(int extraIndent,
+ int skipLevel)
+{
+ TextOutputVisitor *v;
+
+ v = g_malloc0(sizeof(*v));
+
+ v->extraIndent = extraIndent;
+ v->skipLevel = skipLevel;
+ v->string = g_string_new(NULL);
+ v->visitor.type = VISITOR_OUTPUT;
+ v->visitor.type_int64 = print_type_int64;
+ v->visitor.type_uint64 = print_type_uint64;
+ v->visitor.type_size = print_type_size;
+ v->visitor.type_bool = print_type_bool;
+ v->visitor.type_str = print_type_str;
+ v->visitor.type_number = print_type_number;
+ v->visitor.start_list = start_list;
+ v->visitor.next_list = next_list;
+ v->visitor.end_list = end_list;
+ v->visitor.start_struct = start_struct;
+ v->visitor.end_struct = end_struct;
+
+ return v;
+}
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 70ea8ca..72efab8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -115,14 +115,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
Error *err = NULL;
%(c_name)s *tail;
size_t size = sizeof(**obj);
+ size_t element = 0;
- visit_start_list(v, name, (GenericList **)obj, size, &err);
+ visit_start_list(v, name, (GenericList **)obj, size, *obj != NULL, &err);
if (err) {
goto out;
}
for (tail = *obj; tail;
- tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size)) {
+ tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size, ++element)) {
visit_type_%(c_elt_type)s(v, NULL, &tail->value, &err);
if (err) {
break;
--
2.5.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v1 5/6] qapi: generate a qapi_stringify_TYPENAME method for all types
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
` (3 preceding siblings ...)
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types Daniel P. Berrange
@ 2016-06-07 10:11 ` Daniel P. Berrange
2016-06-07 16:23 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 6/6] block: convert to use qapi_stringify_ImageInfoSpecific Daniel P. Berrange
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 10:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth, Eric Blake, Daniel P. Berrange
There are sometimes cases where one might wish to have a
pretty string representation of a QAPI type. For example,
the 'qemu-img info' tool wants to print out ImageInfoSpecific
type in a humand friendly format. Also when debugging problems
in code it is often useful to insert code to print out a QAPI
object.
To address this, add a qapi_stringify_TYPENAME() method for
all types which wraps around the TextOutputVisitor to turn
objects into pretty strings.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
scripts/qapi-types.py | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 437cf6c..c3eef41 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -167,6 +167,40 @@ void qapi_free_%(c_name)s(%(c_name)s *obj)
return ret
+def gen_type_stringify_decl(name):
+ ret = mcgen('''
+
+char *qapi_stringify_%(c_name)s(%(c_name)s *obj, int extraIndent, int skipLevel);
+''',
+ c_name=c_name(name))
+ return ret
+
+
+def gen_type_stringify(name):
+ ret = mcgen('''
+
+char *qapi_stringify_%(c_name)s(%(c_name)s *obj, int extraIndent, int skipLevel)
+{
+ TextOutputVisitor *tov;
+ Visitor *v;
+ char *ret;
+
+ if (!obj) {
+ return NULL;
+ }
+
+ tov = text_output_visitor_new(extraIndent, skipLevel);
+ v = text_output_get_visitor(tov);
+ visit_type_%(c_name)s(v, NULL, &obj, NULL);
+ ret = text_output_get_string(tov);
+ text_output_visitor_cleanup(tov);
+ return ret;
+}
+''',
+ c_name=c_name(name))
+ return ret
+
+
class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def __init__(self):
self.decl = None
@@ -197,6 +231,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.decl += gen_type_cleanup_decl(name)
self.defn += gen_type_cleanup(name)
+ def _gen_type_stringify(self, name):
+ self.decl += gen_type_stringify_decl(name)
+ self.defn += gen_type_stringify(name)
+
def visit_enum_type(self, name, info, values, prefix):
# Special case for our lone builtin enum type
# TODO use something cleaner than existence of info
@@ -215,10 +253,14 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self._btin += gen_type_cleanup_decl(name)
if do_builtins:
self.defn += gen_type_cleanup(name)
+ self._btin += gen_type_stringify_decl(name)
+ if do_builtins:
+ self.defn += gen_type_stringify(name)
else:
self._fwdecl += gen_fwd_object_or_array(name)
self.decl += gen_array(name, element_type)
self._gen_type_cleanup(name)
+ self._gen_type_stringify(name)
def visit_object_type(self, name, info, base, members, variants):
# Nothing to do for the special empty builtin
@@ -233,11 +275,13 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
if not name.startswith('q_'):
# implicit types won't be directly allocated/freed
self._gen_type_cleanup(name)
+ self._gen_type_stringify(name)
def visit_alternate_type(self, name, info, variants):
self._fwdecl += gen_fwd_object_or_array(name)
self.decl += gen_object(name, None, [variants.tag_member], variants)
self._gen_type_cleanup(name)
+ self._gen_type_stringify(name)
# If you link code generated from multiple schemata, you want only one
# instance of the code for built-in types. Generate it only when
@@ -289,6 +333,7 @@ h_comment = '''
fdef.write(mcgen('''
#include "qemu/osdep.h"
#include "qapi/dealloc-visitor.h"
+#include "qapi/text-output-visitor.h"
#include "%(prefix)sqapi-types.h"
#include "%(prefix)sqapi-visit.h"
''',
--
2.5.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v1 6/6] block: convert to use qapi_stringify_ImageInfoSpecific
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
` (4 preceding siblings ...)
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 5/6] qapi: generate a qapi_stringify_TYPENAME method for all types Daniel P. Berrange
@ 2016-06-07 10:11 ` Daniel P. Berrange
2016-06-07 16:59 ` Eric Blake
2016-06-07 12:04 ` [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Eric Blake
2016-06-14 13:56 ` Max Reitz
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 10:11 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth, Eric Blake, Daniel P. Berrange
When 'qemu-img info' prints out format specific information,
it first converts the QAPI object into a JSON based QObject
data structure. Unfortunately structs have to be turned into
dicts, which looses all information about field ordering,
so the data printed appears in a semi-random order.
Convert this to use the qapi_stringify_ImageInfoSpecific()
which uses a visitor to directly pretty-print the objects
without the intermediate QObject conversion, and thus will
maintain struct field ordering.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
block/qapi.c | 101 ++---------------------------------------------------------
1 file changed, 3 insertions(+), 98 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 5594f74..769d7f3 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -29,7 +29,6 @@
#include "block/write-threshold.h"
#include "qmp-commands.h"
#include "qapi-visit.h"
-#include "qapi/qmp-output-visitor.h"
#include "qapi/qmp/types.h"
#include "sysemu/block-backend.h"
#include "qemu/cutils.h"
@@ -599,107 +598,13 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
}
}
-static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
- QDict *dict);
-static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
- QList *list);
-
-static void dump_qobject(fprintf_function func_fprintf, void *f,
- int comp_indent, QObject *obj)
-{
- switch (qobject_type(obj)) {
- case QTYPE_QINT: {
- QInt *value = qobject_to_qint(obj);
- func_fprintf(f, "%" PRId64, qint_get_int(value));
- break;
- }
- case QTYPE_QSTRING: {
- QString *value = qobject_to_qstring(obj);
- func_fprintf(f, "%s", qstring_get_str(value));
- break;
- }
- case QTYPE_QDICT: {
- QDict *value = qobject_to_qdict(obj);
- dump_qdict(func_fprintf, f, comp_indent, value);
- break;
- }
- case QTYPE_QLIST: {
- QList *value = qobject_to_qlist(obj);
- dump_qlist(func_fprintf, f, comp_indent, value);
- break;
- }
- case QTYPE_QFLOAT: {
- QFloat *value = qobject_to_qfloat(obj);
- func_fprintf(f, "%g", qfloat_get_double(value));
- break;
- }
- case QTYPE_QBOOL: {
- QBool *value = qobject_to_qbool(obj);
- func_fprintf(f, "%s", qbool_get_bool(value) ? "true" : "false");
- break;
- }
- default:
- abort();
- }
-}
-
-static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
- QList *list)
-{
- const QListEntry *entry;
- int i = 0;
-
- for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
- QType type = qobject_type(entry->value);
- bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
- func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
- composite ? '\n' : ' ');
- dump_qobject(func_fprintf, f, indentation + 1, entry->value);
- if (!composite) {
- func_fprintf(f, "\n");
- }
- }
-}
-
-static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
- QDict *dict)
-{
- const QDictEntry *entry;
-
- for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
- QType type = qobject_type(entry->value);
- bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
- char *key = g_malloc(strlen(entry->key) + 1);
- int i;
-
- /* replace dashes with spaces in key (variable) names */
- for (i = 0; entry->key[i]; i++) {
- key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
- }
- key[i] = 0;
- func_fprintf(f, "%*s%s:%c", indentation * 4, "", key,
- composite ? '\n' : ' ');
- dump_qobject(func_fprintf, f, indentation + 1, entry->value);
- if (!composite) {
- func_fprintf(f, "\n");
- }
- g_free(key);
- }
-}
void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
ImageInfoSpecific *info_spec)
{
- QmpOutputVisitor *ov = qmp_output_visitor_new();
- QObject *obj, *data;
-
- visit_type_ImageInfoSpecific(qmp_output_get_visitor(ov), NULL, &info_spec,
- &error_abort);
- obj = qmp_output_get_qobject(ov);
- assert(qobject_type(obj) == QTYPE_QDICT);
- data = qdict_get(qobject_to_qdict(obj), "data");
- dump_qobject(func_fprintf, f, 1, data);
- qmp_output_visitor_cleanup(ov);
+ char *str = qapi_stringify_ImageInfoSpecific(info_spec, 4, 2);
+ func_fprintf(f, "%s", str);
+ g_free(str);
}
void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
--
2.5.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
` (5 preceding siblings ...)
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 6/6] block: convert to use qapi_stringify_ImageInfoSpecific Daniel P. Berrange
@ 2016-06-07 12:04 ` Eric Blake
2016-06-07 14:35 ` Daniel P. Berrange
2016-06-14 13:56 ` Max Reitz
7 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2016-06-07 12:04 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]
On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> The 'qemu-img info' tool has ability to print format specific
> information, eg with qcow2 it reports two extra items:
>
> $ qemu-img info ~/VirtualMachines/demo.qcow2
> image: /home/berrange/VirtualMachines/demo.qcow2
> file format: qcow2
> virtual size: 3.0G (3221225472 bytes)
> disk size: 140K
> cluster_size: 65536
> Format specific information:
> compat: 0.10
> refcount bits: 16
>
>
> This is not currently wired up for the LUKS driver. This patch
> series adds that support so that we can report useful data about
> the LUKS volume such as the crypto algorithm choices, key slot
> usage and other volume metadata.
>
> The first patch extends the crypto API to allow querying of the
> format specific metadata
>
> The second patches extends the block API to allow the LUKS driver
> to report the format specific metadata.
Nice!
> uuid: 6ddee74b-3a22-408c-8909-6789d4fa2594
> slots:
> [0]:
> active: true
> iters: 572706
> stripes: 4000
> key-offset: 8
> [1]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 264
> [2]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 520
Is it worth making iters/stripes optional, and present only when active
is true? key-offset, on the other hand, should always be present.
> The remaining 4 patches are improvements to QAPI and the core
> block layer to fix a problem whereby struct fields are output
> in (apparently) random ordering. This is because the QAPI type
> is converted into a QObject for pretty-printing, thus throwing
> away any struct field ordering information.
>
> To address this I created a new TextOutputVisitor which can
> directly pretty-print QAPI types. I then changed the code
> generator to create qapi_stringify_TYPENAME() methods for
> all QAPI types. Finally I changed the block layer over to
> use this stringify approach instead.
Umm, how much does that duplicate my work on adding the JSON pretty
printer, which DOES print JSON in the same order as the underlying QAPI
struct? I still need to address Markus' latest reviews, but suspect we
may be able to combine efforts rather than duplicating.
>
> I'm not sure if QAPI maintainers will find the idea of adding
> qapi_stringify_TYPENAME() methods desirable or not. It is of
> course valid to directly use the TextOutputVisitor from the
> block layer. I felt there might be some use in debugging to
> have a convenient qapi_stringify_TYPENAME() method around
> though - personally I often find it helpful to be able to
> easily dump an QAPI object or any QObject to a humand friendly
> format for debugging and the less code I need write to add
> this temporary debug output the better.
I agree with that sentiment, but being able to dump directly to JSON
without going through an intermediate QObject may already be what you want.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption Daniel P. Berrange
@ 2016-06-07 14:17 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2016-06-07 14:17 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 5281 bytes --]
On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> When creating new block encryption volumes, we accept a list of
> parameters to control the formatting process. It is useful to
> be able to query what those parameters were for existing block
> devices. Add a qcrypto_block_get_info() method which returns a
> QCryptoBlockInfo instance to report this data.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> crypto/block-luks.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++-
> crypto/block.c | 17 +++++++++++++
> crypto/blockpriv.h | 4 +++
> include/crypto/block.h | 16 ++++++++++++
> qapi/crypto.json | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 167 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 17c4300..1c8e4d6 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -201,6 +201,15 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592);
>
> struct QCryptoBlockLUKS {
> QCryptoBlockLUKSHeader header;
> +
> + /* Cache parsed versions of what's in header fields.
s/\./,/
> @@ -947,7 +962,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> }
> hash_alg = QCryptoHashAlgorithm_lookup[luks_opts.hash_alg];
>
> -
> if (strlen(cipher_alg) >= QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN) {
Unrelated cleanup, but I guess it's okay.
> + info->u.luks.hash_alg = luks->hash_alg;
> + info->u.luks.payload_offset = block->payload_offset;
> + info->u.luks.master_key_iters = luks->header.master_key_iterations;
> + info->u.luks.uuid = g_strdup((const char *)luks->header.uuid);
Cast is necessary because the header declared it as uint8_t[]; fair enough.
> +
> + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> + slots = g_new0(QCryptoBlockInfoLUKSSlotList, 1);
> + if (i == 0) {
> + info->u.luks.slots = slots;
> + } else {
> + prev->next = slots;
> + }
> +
> + slots->value = slot = g_new0(QCryptoBlockInfoLUKSSlot, 1);
> + slot->active = luks->header.key_slots[i].active ==
> + QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> + slot->iters = luks->header.key_slots[i].iterations;
> + slot->stripes = luks->header.key_slots[i].stripes;
See my comment on cover letter, on whether iters and stripes need to be
filled out for inactive slots.
> +++ b/include/crypto/block.h
> @@ -138,6 +138,22 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
> void *opaque,
> Error **errp);
>
> +
> +/**
> + * qcrypto_block_get_info:
> + * block:L the block encryption object
stray L
> +++ b/qapi/crypto.json
> @@ -220,3 +220,68 @@
> 'discriminator': 'format',
> 'data': { 'qcow': 'QCryptoBlockOptionsQCow',
> 'luks': 'QCryptoBlockCreateOptionsLUKS' } }
> +
> +
> +##
> +# QCryptoBlockInfoBase:
> +#
> +# The common information that applies to all full disk
> +# encryption formats
> +#
> +# @format: the encryption format
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'QCryptoBlockInfoBase',
> + 'data': { 'format': 'QCryptoBlockFormat' }}
> +
Another candidate for my anonymous union base, once my qapi patches
land. Nothing for you to change now, though.
> +
> +##
> +# QCryptoBlockInfoLUKSSlot:
> +#
> +# Information about the LUKS block encryption key
> +# slot options
> +#
Missing ## terminator and description of the members. Plus a decision on
whether things should be optional.
> +{ 'struct': 'QCryptoBlockInfoLUKSSlot',
> + 'data': {'active': 'bool',
> + 'iters': 'int',
> + 'stripes': 'int',
> + 'key-offset': 'int' } }
> +
> +
> +##
> +# QCryptoBlockInfoLUKS:
> +#
> +# Information about the LUKS block encryption options
> +#
> +# @cipher-alg: the cipher algorithm for data encryption
> +# @cipher-mode: the cipher mode for data encryption
> +# @ivgen-alg: the initialization vector generator
> +# @ivgen-hash-alg: the initialization vector generator hash
Missing #optional marker.
> +# @hash-alg: the master key hash algorithm
Missing docs on payload-offset, master-key-iters, uuid, and slots
> +#
> +# Since: 2.7
> +##
> +{ 'struct': 'QCryptoBlockInfoLUKS',
> + 'data': {'cipher-alg': 'QCryptoCipherAlgorithm',
> + 'cipher-mode': 'QCryptoCipherMode',
> + 'ivgen-alg': 'QCryptoIVGenAlgorithm',
> + '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> + 'hash-alg': 'QCryptoHashAlgorithm',
> + 'payload-offset': 'int',
> + 'master-key-iters': 'int',
> + 'uuid': 'str',
> + 'slots': [ 'QCryptoBlockInfoLUKSSlot' ] }}
> +
> +
> +##
> +# QCryptoBlockInfo:
> +#
> +# Information about the block encryption options
> +#
> +# Since: 2.7
> +##
> +{ 'union': 'QCryptoBlockInfo',
> + 'base': 'QCryptoBlockInfoBase',
> + 'discriminator': 'format',
> + 'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver
2016-06-07 12:04 ` [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Eric Blake
@ 2016-06-07 14:35 ` Daniel P. Berrange
0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 14:35 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth
On Tue, Jun 07, 2016 at 06:04:36AM -0600, Eric Blake wrote:
> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> > uuid: 6ddee74b-3a22-408c-8909-6789d4fa2594
> > slots:
> > [0]:
> > active: true
> > iters: 572706
> > stripes: 4000
> > key-offset: 8
> > [1]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 264
> > [2]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 520
>
> Is it worth making iters/stripes optional, and present only when active
> is true? key-offset, on the other hand, should always be present.
NB stripes is actually fixed at time of creation even if the slot is
not active, but agre we could omit it, not least since it'll be the
same for every slot anyway.
> > The remaining 4 patches are improvements to QAPI and the core
> > block layer to fix a problem whereby struct fields are output
> > in (apparently) random ordering. This is because the QAPI type
> > is converted into a QObject for pretty-printing, thus throwing
> > away any struct field ordering information.
> >
> > To address this I created a new TextOutputVisitor which can
> > directly pretty-print QAPI types. I then changed the code
> > generator to create qapi_stringify_TYPENAME() methods for
> > all QAPI types. Finally I changed the block layer over to
> > use this stringify approach instead.
>
> Umm, how much does that duplicate my work on adding the JSON pretty
> printer, which DOES print JSON in the same order as the underlying QAPI
> struct? I still need to address Markus' latest reviews, but suspect we
> may be able to combine efforts rather than duplicating.
Looking at your code, it seems we've basically got a similar
kind of structure to our visitors, but of course you're using
the qstring routines to format in JSON, where as my visitor
is doing a pretty-plain text formatting that's intended to be
identical to what "qemu-img info" currently reports in plain
text mode.
> > I'm not sure if QAPI maintainers will find the idea of adding
> > qapi_stringify_TYPENAME() methods desirable or not. It is of
> > course valid to directly use the TextOutputVisitor from the
> > block layer. I felt there might be some use in debugging to
> > have a convenient qapi_stringify_TYPENAME() method around
> > though - personally I often find it helpful to be able to
> > easily dump an QAPI object or any QObject to a humand friendly
> > format for debugging and the less code I need write to add
> > this temporary debug output the better.
>
> I agree with that sentiment, but being able to dump directly to JSON
> without going through an intermediate QObject may already be what you want.
We'd have to write something that took the JSON string, parsed it
and then pretty-printed in plain text. This actally sounds like
more work than a visitor that directly outputs pretty plain text.
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 :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info Daniel P. Berrange
@ 2016-06-07 15:36 ` Eric Blake
2016-06-07 15:51 ` Daniel P. Berrange
0 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2016-06-07 15:36 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]
On 06/07/2016 04:11 AM, 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.
>
>
> One somewhat undesirable artifact is that the data fields are
> printed out in (apparantly) random order. This will be addressed
s/apparantly/apparently/
> later by changing the way the block layer pretty-prints the
> image specific data.
Ah, so your new visitor pretty-prints HMP, not JSON. Okay, then I get
to review it after all, and it is probably a good addition in parallel
to my JSON printer.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> block/crypto.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> qapi/block-core.json | 34 +++++++++++++++++++++-
> 2 files changed, 113 insertions(+), 1 deletion(-)
>
> +
> + spec_info = g_new(ImageInfoSpecific, 1);
> + spec_info->type = IMAGE_INFO_SPECIFIC_KIND_LUKS;
> + spec_info->u.luks.data = g_new(ImageInfoSpecificLUKS, 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);
My clone visitor patches would help here.
> +++ b/qapi/block-core.json
> @@ -74,6 +74,37 @@
> 'extents': ['ImageInfo']
> } }
>
> +
> +{ 'struct': 'ImageInfoSpecificLUKSSlot',
> + 'data': {'active': 'bool',
> + 'iters': 'int',
> + 'stripes': 'int',
> + 'key-offset': 'int' } }
Missing documentation, but why do you need it, since it is identical to
QCryptoBlockInfoLUKSSlot in the previous patch?
> +
> +##
> +# @ImageInfoSpecificLUKS:
> +#
> +# @cipher-alg: the cipher algorithm for data encryption
> +# @cipher-mode: the cipher mode for data encryption
> +# @ivgen-alg: the initialization vector generator
> +# @ivgen-hash-alg: the initialization vector generator hash
> +# @hash-alg: the master key hash algorithm
> +#
Not all the members are documented, but isn't this identical to
QCryptoBlockInfoLUKS in the previous patch?
> +# Since: 2.7
> +##
> +{ 'struct': 'ImageInfoSpecificLUKS',
> + 'data': {
> + 'cipher-alg': 'QCryptoCipherAlgorithm',
> + 'cipher-mode': 'QCryptoCipherMode',
> + 'ivgen-alg': 'QCryptoIVGenAlgorithm',
> + '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> + 'hash-alg': 'QCryptoHashAlgorithm',
> + 'payload-offset': 'int',
> + 'master-key-iters': 'int',
> + 'uuid': 'str',
> + 'slots': [ 'ImageInfoSpecificLUKSSlot' ]
> + } }
> +
> ##
> # @ImageInfoSpecific:
> #
> @@ -85,7 +116,8 @@
> { 'union': 'ImageInfoSpecific',
> 'data': {
> 'qcow2': 'ImageInfoSpecificQCow2',
> - 'vmdk': 'ImageInfoSpecificVmdk'
> + 'vmdk': 'ImageInfoSpecificVmdk',
> + 'luks': 'ImageInfoSpecificLUKS'
I guess the difference is whether you are giving the info on a LUKS
image regardless of underlying storage, vs. on a qcow2 image with LUKS
encryption. Still, can't we reuse the type, rather than duplicate it?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks Daniel P. Berrange
@ 2016-06-07 15:40 ` Eric Blake
2016-06-07 15:46 ` Daniel P. Berrange
0 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2016-06-07 15:40 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]
On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> Not all visitor implementations supply the full set of
> visitor callback functions. For example, the string
> output visitor does not provide 'start_struct' and
> friends. If you don't know this and feed it an object
> that uses structs, you'll get a crash:
>
> Segmentation fault (core dumped)
>
> Crashing is fine, because this is a programmer mistake,
> but we can improve the error message upon crash to make
> it obvious what failed by adding assert()s:
>
> qapi/qapi-visit-core.c:32: visit_start_struct: Assertion `v->start_struct != ((void *)0)' failed.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> qapi/qapi-visit-core.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
Up to Markus if he likes this (I think I've proposed the idea, but never
actually written it as a patch, because he implied that the core dump
still points people in the right direction).
>
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index eada467..3b5efbe 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -29,6 +29,7 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
> assert(size);
> assert(v->type != VISITOR_OUTPUT || *obj);
> }
> + assert(v->start_struct != NULL);
I'd have written it assert(v->start_struct) (explicit comparison against
NULL matters in Java, but is just line noise in C).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks
2016-06-07 15:40 ` Eric Blake
@ 2016-06-07 15:46 ` Daniel P. Berrange
0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 15:46 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth
On Tue, Jun 07, 2016 at 09:40:25AM -0600, Eric Blake wrote:
> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> > Not all visitor implementations supply the full set of
> > visitor callback functions. For example, the string
> > output visitor does not provide 'start_struct' and
> > friends. If you don't know this and feed it an object
> > that uses structs, you'll get a crash:
> >
> > Segmentation fault (core dumped)
> >
> > Crashing is fine, because this is a programmer mistake,
> > but we can improve the error message upon crash to make
> > it obvious what failed by adding assert()s:
> >
> > qapi/qapi-visit-core.c:32: visit_start_struct: Assertion `v->start_struct != ((void *)0)' failed.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > qapi/qapi-visit-core.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
>
> Up to Markus if he likes this (I think I've proposed the idea, but never
> actually written it as a patch, because he implied that the core dump
> still points people in the right direction).
I think from the error message I show in the description above
that the new crash is much more "user friendly" - if I saw a
bug report with that assertion message I'd be more likely to
dive in an fix it becasue it's obvious where the issue is,
while a report with just a "Segmentation fault" could be anything.
> > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> > index eada467..3b5efbe 100644
> > --- a/qapi/qapi-visit-core.c
> > +++ b/qapi/qapi-visit-core.c
> > @@ -29,6 +29,7 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
> > assert(size);
> > assert(v->type != VISITOR_OUTPUT || *obj);
> > }
> > + assert(v->start_struct != NULL);
>
> I'd have written it assert(v->start_struct) (explicit comparison against
> NULL matters in Java, but is just line noise in C).
The only reason I included the != NULL, is that it makes
the corresponding assertion message printed a little
more obvious
eg
Assertion `v->start_struct != ((void *)0)' failed.
vs
Assertion `v->start_struct' failed.
Sure, seasoned C programmers will know they're the same, but the less
experianced would likely find the former more obvious at first glance
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 :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info
2016-06-07 15:36 ` Eric Blake
@ 2016-06-07 15:51 ` Daniel P. Berrange
2016-06-07 16:11 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 15:51 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth
On Tue, Jun 07, 2016 at 09:36:08AM -0600, Eric Blake wrote:
> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> > +++ b/qapi/block-core.json
> > @@ -74,6 +74,37 @@
> > 'extents': ['ImageInfo']
> > } }
> >
> > +
> > +{ 'struct': 'ImageInfoSpecificLUKSSlot',
> > + 'data': {'active': 'bool',
> > + 'iters': 'int',
> > + 'stripes': 'int',
> > + 'key-offset': 'int' } }
>
> Missing documentation, but why do you need it, since it is identical to
> QCryptoBlockInfoLUKSSlot in the previous patch?
>
> > +
> > +##
> > +# @ImageInfoSpecificLUKS:
> > +#
> > +# @cipher-alg: the cipher algorithm for data encryption
> > +# @cipher-mode: the cipher mode for data encryption
> > +# @ivgen-alg: the initialization vector generator
> > +# @ivgen-hash-alg: the initialization vector generator hash
> > +# @hash-alg: the master key hash algorithm
> > +#
>
> Not all the members are documented, but isn't this identical to
> QCryptoBlockInfoLUKS in the previous patch?
>
> > +# Since: 2.7
> > +##
> > +{ 'struct': 'ImageInfoSpecificLUKS',
> > + 'data': {
> > + 'cipher-alg': 'QCryptoCipherAlgorithm',
> > + 'cipher-mode': 'QCryptoCipherMode',
> > + 'ivgen-alg': 'QCryptoIVGenAlgorithm',
> > + '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> > + 'hash-alg': 'QCryptoHashAlgorithm',
> > + 'payload-offset': 'int',
> > + 'master-key-iters': 'int',
> > + 'uuid': 'str',
> > + 'slots': [ 'ImageInfoSpecificLUKSSlot' ]
> > + } }
> > +
> > ##
> > # @ImageInfoSpecific:
> > #
> > @@ -85,7 +116,8 @@
> > { 'union': 'ImageInfoSpecific',
> > 'data': {
> > 'qcow2': 'ImageInfoSpecificQCow2',
> > - 'vmdk': 'ImageInfoSpecificVmdk'
> > + 'vmdk': 'ImageInfoSpecificVmdk',
> > + 'luks': 'ImageInfoSpecificLUKS'
>
> I guess the difference is whether you are giving the info on a LUKS
> image regardless of underlying storage, vs. on a qcow2 image with LUKS
> encryption. Still, can't we reuse the type, rather than duplicate it?
Essentially yes, and this is something I meant to mention in
the cover letter.
I wasn't really sure on the best approach to take here. I
could certainly re-use the QCrypto QAPI object by doing
{ 'union': 'ImageInfoSpecific',
'data': {
'qcow2': 'ImageInfoSpecificQCow2',
'vmdk': 'ImageInfoSpecificVmdk',
'luks': 'QCryptoBlockInfoLUKS',
} }
I was not sure if that was a good idea or whether it is better
to have isolation between the crypto layer and block layer, as
safety net in case we need them to diverge. The main thing was
whether the data we report from the block driver will need to
include extra stuff not present in QCryptoBlockInfoLUKS, perhaps
related to the backing file/format.
I guess another option would be for ImageInfoSpecificLUKS
to sub-class QCryptoBlockInfoLUKS in that case.
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 :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types Daniel P. Berrange
@ 2016-06-07 16:09 ` Eric Blake
2016-06-07 16:20 ` Daniel P. Berrange
0 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2016-06-07 16:09 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 10712 bytes --]
On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> The current approach for pretty-printing QAPI types is to
> convert them to JSON using the QMP output visitor and then
> pretty-print the JSON document. This has an unfixable problem
> that structs get their keys printed out in random order, since
> JSON dicts do not contain any key ordering information.
>
> To address this, introduce a text output visitor that can
> directly pretty print a QAPI type into a string.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> include/qapi/text-output-visitor.h | 73 ++++++++++++
> include/qapi/visitor-impl.h | 5 +-
> include/qapi/visitor.h | 5 +-
> qapi/Makefile.objs | 1 +
> qapi/opts-visitor.c | 5 +-
> qapi/qapi-dealloc-visitor.c | 4 +-
> qapi/qapi-visit-core.c | 9 +-
> qapi/qmp-input-visitor.c | 5 +-
> qapi/qmp-output-visitor.c | 4 +-
> qapi/string-input-visitor.c | 5 +-
> qapi/string-output-visitor.c | 5 +-
Why can't we enhance the existing string-output-visitor to handle structs?
> qapi/text-output-visitor.c | 235 +++++++++++++++++++++++++++++++++++++
> scripts/qapi-visit.py | 5 +-
Missing a testsuite addition.
> 13 files changed, 339 insertions(+), 22 deletions(-)
> create mode 100644 include/qapi/text-output-visitor.h
> create mode 100644 qapi/text-output-visitor.c
>
> diff --git a/include/qapi/text-output-visitor.h b/include/qapi/text-output-visitor.h
> new file mode 100644
> index 0000000..3b742b7
> --- /dev/null
> +++ b/include/qapi/text-output-visitor.h
> @@ -0,0 +1,73 @@
> +TextOutputVisitor *text_output_visitor_new(int extraIndent,
> + int skipLevel);
Conflicts with the work in my JSON/cloning visitor series, which tries
to hide all visitors behind a polymorphic interface so that we don't
need to expose the subtype TextOutputVisitor to the caller.
> +++ b/include/qapi/visitor-impl.h
> @@ -52,10 +52,11 @@ struct Visitor
> /* Must be set; implementations may require @list to be non-null,
> * but must document it. */
> void (*start_list)(Visitor *v, const char *name, GenericList **list,
> - size_t size, Error **errp);
> + size_t size, bool hasElements, Error **errp);
Ah, you had to touch ALL the visitors, to add hasElements. What are its
semantics on an input list, where you don't know if you have elements
until after calling visit_start_list()? If we DO need this parameter,
it should be a separate patch to add it in, then the patch to create the
new visitor. But I'm not convinced we need it: *list already serves
that purpose (if *list is NULL, you have no elements; if it is non-NULL,
then you have at least one element; and if the list is optional, then
the call to visit_type_optional() already tells you whether the
[possibly-empty] list is even present).
>
> /* Must be set */
> - GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
> + GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size,
> + size_t element);
Why do we need to list the element size twice? Or is one the size of
the GenericList object wrapping the element? I'm still not convinced we
need the size of an element at this point in the visit.
> +++ b/include/qapi/visitor.h
> @@ -303,7 +303,7 @@ void visit_end_struct(Visitor *v);
Missing documentation about the new visitor in the up-front summary
documentation.
> * even if intermediate visits fail. See the examples above.
> */
> void visit_start_list(Visitor *v, const char *name, GenericList **list,
> - size_t size, Error **errp);
> + size_t size, bool hasElements, Error **errp);
Missing documentation on what the new parameter represents.
>
> /*
> * Iterate over a GenericList during a non-virtual list visit.
> @@ -319,7 +319,8 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list,
> * the list, with that function's name parameter set to NULL and obj
> * set to the address of @tail->value.
> */
> -GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
> +GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size,
> + size_t element);
>
Missing documentation on what the new parameter represents.
> +++ b/qapi/text-output-visitor.c
> @@ -0,0 +1,235 @@
> +/*
> + * Text pretty printing Visitor
> + *
> + * Copyright Red Hat, Inc. 2016
> + *
> + * Author: Daniel Berrange <berrange@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qapi/text-output-visitor.h"
> +#include "qapi/visitor-impl.h"
> +#include <math.h>
> +
> +struct TextOutputVisitor {
> + Visitor visitor;
> + GString *string;
> + int level;
> + int skipLevel;
> + int extraIndent;
> +};
> +
> +static TextOutputVisitor *to_sov(Visitor *v)
> +{
> + return container_of(v, TextOutputVisitor, visitor);
> +}
> +
> +
> +#define INDENT (sov->extraIndent + ((sov->level - sov->skipLevel) * 4))
> +
> +static void print_type_int64(Visitor *v, const char *name, int64_t *obj,
> + Error **errp)
> +{
> + TextOutputVisitor *sov = to_sov(v);
> +
> + if (sov->level < sov->skipLevel) {
> + return;
> + }
> + g_string_append_printf(sov->string,
> + "%*s%s: %" PRIu64 "\n",
> + INDENT, "",
> + name, *obj);
name is NULL during a list visit, and directly printing NULL through %s
is a glibc extension, liable to crash elsewhere.
> +}
> +
> +static void print_type_uint64(Visitor *v, const char *name, uint64_t *obj,
> + Error **errp)
> +{
> + TextOutputVisitor *sov = to_sov(v);
> +
> + if (sov->level < sov->skipLevel) {
> + return;
> + }
> + g_string_append_printf(sov->string,
> + "%*s%s: %" PRIi64 "\n",
> + INDENT, "",
> + name, *obj);
And again. You may want to copy ideas from my JSON output visitor, for
having common code that outputs the indentation and name (when present),
rather than duplicating the common part in every callback.
> +}
> +
> +static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
> + Error **errp)
> +{
> + TextOutputVisitor *sov = to_sov(v);
> + static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
Don't we already have a util/ function for pretty-printing a size? In
fact, doesn't the existing StringOutputVisitor have code for doing it?
> +static void print_type_number(Visitor *v, const char *name, double *obj,
> + Error **errp)
> +{
> + TextOutputVisitor *sov = to_sov(v);
> +
> + if (sov->level < sov->skipLevel) {
> + return;
> + }
> + g_string_append_printf(sov->string,
> + "%*s%s: %f\n",
I guess %f is okay for human output (we already have documented bugs to
fix in our QMP output visitors that a default of %f loses precision).
> +static void
> +start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> + bool hasElements, Error **errp)
> +{
> + TextOutputVisitor *sov = to_sov(v);
> +
> + if (sov->level >= sov->skipLevel) {
> + g_string_append_printf(sov->string,
> + "%*s%s:\n",
> + INDENT, "",
> + name);
> + }
> + sov->level++;
> + if (hasElements) {
Again, I think hasElements is redundant; you can use *list == NULL as a
witness of whether there are elements.
> + if (sov->level >= sov->skipLevel) {
> + g_string_append_printf(sov->string,
> + "%*s[0]:\n",
> + INDENT, "");
> + }
> + }
> +}
> +
> +static GenericList *next_list(Visitor *v, GenericList *tail,
> + size_t size, size_t element)
> +{
> + TextOutputVisitor *sov = to_sov(v);
> + GenericList *ret = tail->next;
> + if (ret && sov->level >= sov->skipLevel) {
> + g_string_append_printf(sov->string,
> + "%*s[%zu]:\n",
> + INDENT, "", element);
Oooooh, I see. You're using 'element' as the index within the array,
not as a size. I think naming it 'idx' (or 'index', if that doesn't
cause older compilers to barf for shadowing a function name), would make
more sense, but it definitely highlights the need for documentation.
Also, why does element 0 have to be special-cased? Is there a way to
get the element number embedded into the 'name' parameter, rather than
our current practice of passing NULL for the name? Is there any way to
track the state in a stack, rather than making the caller pass in a new
parameter, so that printing the index number is localized to this
visitor rather than touching the interface to every other visitor?
> +++ b/scripts/qapi-visit.py
> @@ -115,14 +115,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
> Error *err = NULL;
> %(c_name)s *tail;
> size_t size = sizeof(**obj);
> + size_t element = 0;
>
> - visit_start_list(v, name, (GenericList **)obj, size, &err);
> + visit_start_list(v, name, (GenericList **)obj, size, *obj != NULL, &err);
This deferences *obj unconditionally, which may not be safe for input
visitors.
> if (err) {
> goto out;
> }
>
> for (tail = *obj; tail;
> - tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size)) {
> + tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size, ++element)) {
Okay, this change may be independently useful, but again, I think
'index' (or 'idx') might be a nicer name than 'element' to convey what
it is tracking. I also wonder if embedding it into 'name' makes more sense.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info
2016-06-07 15:51 ` Daniel P. Berrange
@ 2016-06-07 16:11 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2016-06-07 16:11 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]
On 06/07/2016 09:51 AM, Daniel P. Berrange wrote:
>>
>> Missing documentation, but why do you need it, since it is identical to
>> QCryptoBlockInfoLUKSSlot in the previous patch?
>>
>
> Essentially yes, and this is something I meant to mention in
> the cover letter.
>
> I wasn't really sure on the best approach to take here. I
> could certainly re-use the QCrypto QAPI object by doing
>
> { 'union': 'ImageInfoSpecific',
> 'data': {
> 'qcow2': 'ImageInfoSpecificQCow2',
> 'vmdk': 'ImageInfoSpecificVmdk',
> 'luks': 'QCryptoBlockInfoLUKS',
> } }
>
> I was not sure if that was a good idea or whether it is better
> to have isolation between the crypto layer and block layer, as
> safety net in case we need them to diverge.
If we need to diverge in the future, then we can create the new type at
that time. We intentionally made introspection hide type names, so that
we are not held hostage by type name changes, and so that splitting
types to add functionality to one but not all uses is safe.
> The main thing was
> whether the data we report from the block driver will need to
> include extra stuff not present in QCryptoBlockInfoLUKS, perhaps
> related to the backing file/format.
That may be an argument for making one type the base class for the
other, rather than a complete reimplementation.
>
> I guess another option would be for ImageInfoSpecificLUKS
> to sub-class QCryptoBlockInfoLUKS in that case.
Yep, just what I said :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types
2016-06-07 16:09 ` Eric Blake
@ 2016-06-07 16:20 ` Daniel P. Berrange
2016-06-07 16:40 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 16:20 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth
On Tue, Jun 07, 2016 at 10:09:48AM -0600, Eric Blake wrote:
> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> > The current approach for pretty-printing QAPI types is to
> > convert them to JSON using the QMP output visitor and then
> > pretty-print the JSON document. This has an unfixable problem
> > that structs get their keys printed out in random order, since
> > JSON dicts do not contain any key ordering information.
> >
> > To address this, introduce a text output visitor that can
> > directly pretty print a QAPI type into a string.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > include/qapi/text-output-visitor.h | 73 ++++++++++++
> > include/qapi/visitor-impl.h | 5 +-
> > include/qapi/visitor.h | 5 +-
> > qapi/Makefile.objs | 1 +
> > qapi/opts-visitor.c | 5 +-
> > qapi/qapi-dealloc-visitor.c | 4 +-
> > qapi/qapi-visit-core.c | 9 +-
> > qapi/qmp-input-visitor.c | 5 +-
> > qapi/qmp-output-visitor.c | 4 +-
> > qapi/string-input-visitor.c | 5 +-
> > qapi/string-output-visitor.c | 5 +-
>
> Why can't we enhance the existing string-output-visitor to handle structs?
string-output-visitor seems to be doing something very
different from this. In particular it only ever seems
to output the values, never the field names. So if we
did enhance string-output-visitor, we'd basically have
to make all of its code conditional to output in one
style or the other style, at which point I didn't think
it was really buying us anything vs a new visitor.
> > +++ b/include/qapi/visitor-impl.h
> > @@ -52,10 +52,11 @@ struct Visitor
> > /* Must be set; implementations may require @list to be non-null,
> > * but must document it. */
> > void (*start_list)(Visitor *v, const char *name, GenericList **list,
> > - size_t size, Error **errp);
> > + size_t size, bool hasElements, Error **errp);
>
> Ah, you had to touch ALL the visitors, to add hasElements. What are its
> semantics on an input list, where you don't know if you have elements
> until after calling visit_start_list()? If we DO need this parameter,
> it should be a separate patch to add it in, then the patch to create the
> new visitor. But I'm not convinced we need it: *list already serves
> that purpose (if *list is NULL, you have no elements; if it is non-NULL,
> then you have at least one element; and if the list is optional, then
> the call to visit_type_optional() already tells you whether the
> [possibly-empty] list is even present).
Yep, I didn't realize that *list could be used to infer this.
This addition is clearly redundant based on that.
>
> >
> > /* Must be set */
> > - GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
> > + GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size,
> > + size_t element);
>
> Why do we need to list the element size twice? Or is one the size of
> the GenericList object wrapping the element? I'm still not convinced we
> need the size of an element at this point in the visit.
'size_t element' isn't the size of an element, it is the really
the list index - eg 1, 2, 3, 4, etc. I didn't call it 'index'
because that causes a clashing symbol, but I guess I could
have used 'idx' instead.
> > +++ b/qapi/text-output-visitor.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * Text pretty printing Visitor
> > + *
> > + * Copyright Red Hat, Inc. 2016
> > + *
> > + * Author: Daniel Berrange <berrange@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu-common.h"
> > +#include "qapi/text-output-visitor.h"
> > +#include "qapi/visitor-impl.h"
> > +#include <math.h>
> > +
> > +struct TextOutputVisitor {
> > + Visitor visitor;
> > + GString *string;
> > + int level;
> > + int skipLevel;
> > + int extraIndent;
> > +};
> > +
> > +static TextOutputVisitor *to_sov(Visitor *v)
> > +{
> > + return container_of(v, TextOutputVisitor, visitor);
> > +}
> > +
> > +
> > +#define INDENT (sov->extraIndent + ((sov->level - sov->skipLevel) * 4))
> > +
> > +static void print_type_int64(Visitor *v, const char *name, int64_t *obj,
> > + Error **errp)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > +
> > + if (sov->level < sov->skipLevel) {
> > + return;
> > + }
> > + g_string_append_printf(sov->string,
> > + "%*s%s: %" PRIu64 "\n",
> > + INDENT, "",
> > + name, *obj);
>
> name is NULL during a list visit, and directly printing NULL through %s
> is a glibc extension, liable to crash elsewhere.
Oh of course - I only tested where the list elements were themselves
structs, so missed that nuance.
> > +static void print_type_uint64(Visitor *v, const char *name, uint64_t *obj,
> > + Error **errp)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > +
> > + if (sov->level < sov->skipLevel) {
> > + return;
> > + }
> > + g_string_append_printf(sov->string,
> > + "%*s%s: %" PRIi64 "\n",
> > + INDENT, "",
> > + name, *obj);
>
> And again. You may want to copy ideas from my JSON output visitor, for
> having common code that outputs the indentation and name (when present),
> rather than duplicating the common part in every callback.
>
> > +}
> > +
> > +static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
> > + Error **errp)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > + static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
>
> Don't we already have a util/ function for pretty-printing a size? In
> fact, doesn't the existing StringOutputVisitor have code for doing it?
Guess where this code was copied from - StringOutputVisitor :-) If this
idea is amenable in general, I'll go back and cleanup this patch be better.
> > +static GenericList *next_list(Visitor *v, GenericList *tail,
> > + size_t size, size_t element)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > + GenericList *ret = tail->next;
> > + if (ret && sov->level >= sov->skipLevel) {
> > + g_string_append_printf(sov->string,
> > + "%*s[%zu]:\n",
> > + INDENT, "", element);
>
> Oooooh, I see. You're using 'element' as the index within the array,
> not as a size. I think naming it 'idx' (or 'index', if that doesn't
> cause older compilers to barf for shadowing a function name), would make
> more sense, but it definitely highlights the need for documentation.
> Also, why does element 0 have to be special-cased?
The pattern of calling means 'next_list' is invoked /after/
each element is visited, but we want to print out the header
/before/ each element. So I had to special case 0, and also
use the 'if(ret)' check to skip printing a header after the
last element.
> Is there a way to
> get the element number embedded into the 'name' parameter, rather than
> our current practice of passing NULL for the name? Is there any way to
> track the state in a stack, rather than making the caller pass in a new
> parameter, so that printing the index number is localized to this
> visitor rather than touching the interface to every other visitor?
That's a potential approach I can look at to avoid adding the
extra parameter. It would still need the first/last special
case. I'm totally open minded about solutions really - this
was simply the quickest possible approach to demonstrate the
problem i was solving. Happy to redo it with any better
approach
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 :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 5/6] qapi: generate a qapi_stringify_TYPENAME method for all types
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 5/6] qapi: generate a qapi_stringify_TYPENAME method for all types Daniel P. Berrange
@ 2016-06-07 16:23 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2016-06-07 16:23 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 916 bytes --]
On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> There are sometimes cases where one might wish to have a
> pretty string representation of a QAPI type. For example,
> the 'qemu-img info' tool wants to print out ImageInfoSpecific
> type in a humand friendly format. Also when debugging problems
> in code it is often useful to insert code to print out a QAPI
> object.
>
> To address this, add a qapi_stringify_TYPENAME() method for
> all types which wraps around the TextOutputVisitor to turn
> objects into pretty strings.
It might be nicer to copy the approach I used for the clone visitor,
with a QAPI_CLONE() macro that forwards to a special-case helper
(including necessary type-punning), rather than making a much larger .h
with lots of declarations that aren't actually used.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types
2016-06-07 16:20 ` Daniel P. Berrange
@ 2016-06-07 16:40 ` Eric Blake
2016-06-07 16:45 ` Daniel P. Berrange
0 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2016-06-07 16:40 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4439 bytes --]
On 06/07/2016 10:20 AM, Daniel P. Berrange wrote:
> On Tue, Jun 07, 2016 at 10:09:48AM -0600, Eric Blake wrote:
>> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
>>> The current approach for pretty-printing QAPI types is to
>>> convert them to JSON using the QMP output visitor and then
>>> pretty-print the JSON document. This has an unfixable problem
>>> that structs get their keys printed out in random order, since
>>> JSON dicts do not contain any key ordering information.
>>>
>>> To address this, introduce a text output visitor that can
>>> directly pretty print a QAPI type into a string.
>>>
>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>> ---
>>> include/qapi/text-output-visitor.h | 73 ++++++++++++
>>> include/qapi/visitor-impl.h | 5 +-
>>> include/qapi/visitor.h | 5 +-
>>> qapi/Makefile.objs | 1 +
>>> qapi/opts-visitor.c | 5 +-
>>> qapi/qapi-dealloc-visitor.c | 4 +-
>>> qapi/qapi-visit-core.c | 9 +-
>>> qapi/qmp-input-visitor.c | 5 +-
>>> qapi/qmp-output-visitor.c | 4 +-
>>> qapi/string-input-visitor.c | 5 +-
>>> qapi/string-output-visitor.c | 5 +-
>>
>> Why can't we enhance the existing string-output-visitor to handle structs?
>
> string-output-visitor seems to be doing something very
> different from this. In particular it only ever seems
> to output the values, never the field names. So if we
> did enhance string-output-visitor, we'd basically have
> to make all of its code conditional to output in one
> style or the other style, at which point I didn't think
> it was really buying us anything vs a new visitor.
That is, it was always doing a top-level visit of a scalar or array of
scalars, and nothing else. It may still be something that can be merged.
Maybe I should take a rough shot at it, since I have ideas on how to use
a common handler for name/list index (and do nothing at the top level),
then the rest of each callback is independent from what name prefix, if
any, was output. On the other hand, I guess the way intList is handled
(compacting it into a single list, instead of each element of the list),
may indeed be a reason to keep it as two visitors.
>> Why do we need to list the element size twice? Or is one the size of
>> the GenericList object wrapping the element? I'm still not convinced we
>> need the size of an element at this point in the visit.
>
> 'size_t element' isn't the size of an element, it is the really
> the list index - eg 1, 2, 3, 4, etc. I didn't call it 'index'
> because that causes a clashing symbol, but I guess I could
> have used 'idx' instead.
The perils of replying to email text in the order I read it. I think I
figured that out later on. And it is indeed annoying that older gcc
warns about conflicts with shadowing 'index'.
>> Don't we already have a util/ function for pretty-printing a size? In
>> fact, doesn't the existing StringOutputVisitor have code for doing it?
>
> Guess where this code was copied from - StringOutputVisitor :-) If this
> idea is amenable in general, I'll go back and cleanup this patch be better.
A common helper may be nice, if we have two visitors both wanting to use it.
>>
>> Oooooh, I see. You're using 'element' as the index within the array,
>> not as a size. I think naming it 'idx' (or 'index', if that doesn't
>> cause older compilers to barf for shadowing a function name), would make
>> more sense, but it definitely highlights the need for documentation.
>> Also, why does element 0 have to be special-cased?
>
> The pattern of calling means 'next_list' is invoked /after/
> each element is visited, but we want to print out the header
> /before/ each element. So I had to special case 0, and also
> use the 'if(ret)' check to skip printing a header after the
> last element.
But if we have a common helper function (see json_output_name() in my
JSON series), then that helper will be called before every element, and
can easily tell whether we are in struct context (name is non-null,
print it) or list context (name is NULL, print our current counter then
increment it), rather than having to special case the code around the
elements.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types
2016-06-07 16:40 ` Eric Blake
@ 2016-06-07 16:45 ` Daniel P. Berrange
0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-07 16:45 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster,
Michael Roth
On Tue, Jun 07, 2016 at 10:40:36AM -0600, Eric Blake wrote:
> On 06/07/2016 10:20 AM, Daniel P. Berrange wrote:
> > On Tue, Jun 07, 2016 at 10:09:48AM -0600, Eric Blake wrote:
> >> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> >>> The current approach for pretty-printing QAPI types is to
> >>> convert them to JSON using the QMP output visitor and then
> >>> pretty-print the JSON document. This has an unfixable problem
> >>> that structs get their keys printed out in random order, since
> >>> JSON dicts do not contain any key ordering information.
> >>>
> >>> To address this, introduce a text output visitor that can
> >>> directly pretty print a QAPI type into a string.
> >>>
> >>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >>> ---
> >>> include/qapi/text-output-visitor.h | 73 ++++++++++++
> >>> include/qapi/visitor-impl.h | 5 +-
> >>> include/qapi/visitor.h | 5 +-
> >>> qapi/Makefile.objs | 1 +
> >>> qapi/opts-visitor.c | 5 +-
> >>> qapi/qapi-dealloc-visitor.c | 4 +-
> >>> qapi/qapi-visit-core.c | 9 +-
> >>> qapi/qmp-input-visitor.c | 5 +-
> >>> qapi/qmp-output-visitor.c | 4 +-
> >>> qapi/string-input-visitor.c | 5 +-
> >>> qapi/string-output-visitor.c | 5 +-
> >>
> >> Why can't we enhance the existing string-output-visitor to handle structs?
> >
> > string-output-visitor seems to be doing something very
> > different from this. In particular it only ever seems
> > to output the values, never the field names. So if we
> > did enhance string-output-visitor, we'd basically have
> > to make all of its code conditional to output in one
> > style or the other style, at which point I didn't think
> > it was really buying us anything vs a new visitor.
>
> That is, it was always doing a top-level visit of a scalar or array of
> scalars, and nothing else. It may still be something that can be merged.
> Maybe I should take a rough shot at it, since I have ideas on how to use
> a common handler for name/list index (and do nothing at the top level),
> then the rest of each callback is independent from what name prefix, if
> any, was output. On the other hand, I guess the way intList is handled
> (compacting it into a single list, instead of each element of the list),
> may indeed be a reason to keep it as two visitors.
If you want to have a shot, go ahead. I put this patch at the end of my
series, since it wasn't a blocking issue to resolve the struct field
ordering, just a "nice to have", so no rush.
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 :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 6/6] block: convert to use qapi_stringify_ImageInfoSpecific
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 6/6] block: convert to use qapi_stringify_ImageInfoSpecific Daniel P. Berrange
@ 2016-06-07 16:59 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2016-06-07 16:59 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Max Reitz, Markus Armbruster, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]
On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
> When 'qemu-img info' prints out format specific information,
> it first converts the QAPI object into a JSON based QObject
> data structure. Unfortunately structs have to be turned into
> dicts, which looses all information about field ordering,
s/looses/loses/
> so the data printed appears in a semi-random order.
>
> Convert this to use the qapi_stringify_ImageInfoSpecific()
> which uses a visitor to directly pretty-print the objects
> without the intermediate QObject conversion, and thus will
> maintain struct field ordering.
>
The idea makes sense. I'm not sure we want to go all the way to
qapi_stringify_ImageInfoSpecific(), vs. just having a nice accessor
macro that takes care of type-punning through a common actor (the way I
did it in my series for QAPI_CLONE()).
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> block/qapi.c | 101 ++---------------------------------------------------------
> 1 file changed, 3 insertions(+), 98 deletions(-)
>
> @@ -599,107 +598,13 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
> }
> }
>
> -static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
> - QDict *dict);
> -static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
> - QList *list);
> -
> -static void dump_qobject(fprintf_function func_fprintf, void *f,
> - int comp_indent, QObject *obj)
> -{
> - switch (qobject_type(obj)) {
> - case QTYPE_QINT: {
> - QInt *value = qobject_to_qint(obj);
> - func_fprintf(f, "%" PRId64, qint_get_int(value));
> - break;
It's also nice that we're moving some of this code into a more
general-purpose visitor.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
` (6 preceding siblings ...)
2016-06-07 12:04 ` [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Eric Blake
@ 2016-06-14 13:56 ` Max Reitz
2016-06-14 14:05 ` Daniel P. Berrange
7 siblings, 1 reply; 23+ messages in thread
From: Max Reitz @ 2016-06-14 13:56 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
Cc: qemu-block, Kevin Wolf, Markus Armbruster, Michael Roth, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 3396 bytes --]
On 07.06.2016 12:11, Daniel P. Berrange wrote:
> The 'qemu-img info' tool has ability to print format specific
> information, eg with qcow2 it reports two extra items:
>
> $ qemu-img info ~/VirtualMachines/demo.qcow2
> image: /home/berrange/VirtualMachines/demo.qcow2
> file format: qcow2
> virtual size: 3.0G (3221225472 bytes)
> disk size: 140K
> cluster_size: 65536
> Format specific information:
> compat: 0.10
> refcount bits: 16
>
>
> This is not currently wired up for the LUKS driver. This patch
> series adds that support so that we can report useful data about
> the LUKS volume such as the crypto algorithm choices, key slot
> usage and other volume metadata.
>
> The first patch extends the crypto API to allow querying of the
> format specific metadata
>
> The second patches extends the block API to allow the LUKS driver
> to report the format specific metadata.
>
> $ 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:
> cipher-alg: aes-128
> cipher-mode: xts
> ivgen-alg: plain64
> hash-alg: sha1
> payload-offset: 2097152
> master-key-iters: 142375
> uuid: 6ddee74b-3a22-408c-8909-6789d4fa2594
> slots:
> [0]:
> active: true
> iters: 572706
> stripes: 4000
> key-offset: 8
> [1]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 264
> [2]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 520
> [3]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 776
> [4]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 1032
> [5]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 1288
> [6]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 1544
> [7]:
> active: false
> iters: 0
> stripes: 4000
> key-offset: 1800
>
> The remaining 4 patches are improvements to QAPI and the core
> block layer to fix a problem whereby struct fields are output
> in (apparently) random ordering. This is because the QAPI type
> is converted into a QObject for pretty-printing, thus throwing
> away any struct field ordering information.
>
> To address this I created a new TextOutputVisitor which can
> directly pretty-print QAPI types. I then changed the code
> generator to create qapi_stringify_TYPENAME() methods for
> all QAPI types. Finally I changed the block layer over to
> use this stringify approach instead.
General nagging: This new approach no longer replaces dashes with spaces
in the key names. Is it worth doing something about this?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver
2016-06-14 13:56 ` Max Reitz
@ 2016-06-14 14:05 ` Daniel P. Berrange
0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2016-06-14 14:05 UTC (permalink / raw)
To: Max Reitz
Cc: qemu-devel, qemu-block, Kevin Wolf, Markus Armbruster,
Michael Roth, Eric Blake
On Tue, Jun 14, 2016 at 03:56:30PM +0200, Max Reitz wrote:
> On 07.06.2016 12:11, Daniel P. Berrange wrote:
> > The 'qemu-img info' tool has ability to print format specific
> > information, eg with qcow2 it reports two extra items:
> >
> > $ qemu-img info ~/VirtualMachines/demo.qcow2
> > image: /home/berrange/VirtualMachines/demo.qcow2
> > file format: qcow2
> > virtual size: 3.0G (3221225472 bytes)
> > disk size: 140K
> > cluster_size: 65536
> > Format specific information:
> > compat: 0.10
> > refcount bits: 16
> >
> >
> > This is not currently wired up for the LUKS driver. This patch
> > series adds that support so that we can report useful data about
> > the LUKS volume such as the crypto algorithm choices, key slot
> > usage and other volume metadata.
> >
> > The first patch extends the crypto API to allow querying of the
> > format specific metadata
> >
> > The second patches extends the block API to allow the LUKS driver
> > to report the format specific metadata.
> >
> > $ 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:
> > cipher-alg: aes-128
> > cipher-mode: xts
> > ivgen-alg: plain64
> > hash-alg: sha1
> > payload-offset: 2097152
> > master-key-iters: 142375
> > uuid: 6ddee74b-3a22-408c-8909-6789d4fa2594
> > slots:
> > [0]:
> > active: true
> > iters: 572706
> > stripes: 4000
> > key-offset: 8
> > [1]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 264
> > [2]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 520
> > [3]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 776
> > [4]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 1032
> > [5]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 1288
> > [6]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 1544
> > [7]:
> > active: false
> > iters: 0
> > stripes: 4000
> > key-offset: 1800
> >
> > The remaining 4 patches are improvements to QAPI and the core
> > block layer to fix a problem whereby struct fields are output
> > in (apparently) random ordering. This is because the QAPI type
> > is converted into a QObject for pretty-printing, thus throwing
> > away any struct field ordering information.
> >
> > To address this I created a new TextOutputVisitor which can
> > directly pretty-print QAPI types. I then changed the code
> > generator to create qapi_stringify_TYPENAME() methods for
> > all QAPI types. Finally I changed the block layer over to
> > use this stringify approach instead.
>
> General nagging: This new approach no longer replaces dashes with spaces
> in the key names. Is it worth doing something about this?
Opps, didn't notice that. Should be pretty easy to deal with that
in the visitor.
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 :|
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-06-14 14:06 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 10:11 [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Daniel P. Berrange
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 1/6] crypto: add support for querying parameters for block encryption Daniel P. Berrange
2016-06-07 14:17 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info Daniel P. Berrange
2016-06-07 15:36 ` Eric Blake
2016-06-07 15:51 ` Daniel P. Berrange
2016-06-07 16:11 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 3/6] qapi: assert that visitor impls have required callbacks Daniel P. Berrange
2016-06-07 15:40 ` Eric Blake
2016-06-07 15:46 ` Daniel P. Berrange
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types Daniel P. Berrange
2016-06-07 16:09 ` Eric Blake
2016-06-07 16:20 ` Daniel P. Berrange
2016-06-07 16:40 ` Eric Blake
2016-06-07 16:45 ` Daniel P. Berrange
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 5/6] qapi: generate a qapi_stringify_TYPENAME method for all types Daniel P. Berrange
2016-06-07 16:23 ` Eric Blake
2016-06-07 10:11 ` [Qemu-devel] [PATCH v1 6/6] block: convert to use qapi_stringify_ImageInfoSpecific Daniel P. Berrange
2016-06-07 16:59 ` Eric Blake
2016-06-07 12:04 ` [Qemu-devel] [PATCH v1 0/6] Report format specific info for LUKS block driver Eric Blake
2016-06-07 14:35 ` Daniel P. Berrange
2016-06-14 13:56 ` Max Reitz
2016-06-14 14:05 ` Daniel P. Berrange
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.