* [Qemu-devel] [PATCH RFC v3 01/20] qapi: use 'type' in generated C code to match QMP union wire form
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 02/20] vnc: hoist allocation of VncBasicInfo to callers Eric Blake
` (18 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, Eduardo Habkost, open list:qcow2,
Michael S. Tsirkin, Jason Wang, armbru, Luiz Capitulino,
Gerd Hoffmann, Stefan Hajnoczi, Paolo Bonzini, Igor Mammedov,
Michael Roth
When dealing with simple qapi unions, the code was generating a
discriminator field of 'kind' even though the discriminator is
sent as 'type' over QMP. Renaming things to match gets us one
step closer to reusing common generator code for both simple and
flat unions, without having to special case the naming choice
for simple unions.
However, this patch does not rename the generated enum, which is
still 'unionnameKind'; if we wanted, a further patch could
generate implicit enums as 'unionnameType', with even more
churn to C code to react, and probably update the qapi generator
to reserve the 'fooType' namespace instead of 'fooKind'. But
that is a lot harder, as we already have existing qapi usage
of types that end in 'Type'.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.c | 2 +-
block/vmdk.c | 2 +-
blockdev.c | 16 ++++++++--------
hmp.c | 12 ++++++------
hw/input/hid.c | 2 +-
hw/input/ps2.c | 2 +-
hw/input/virtio-input-hid.c | 2 +-
hw/mem/pc-dimm.c | 2 +-
net/dump.c | 2 +-
net/hub.c | 2 +-
net/l2tpv3.c | 2 +-
net/net.c | 20 ++++++++++----------
net/slirp.c | 2 +-
net/socket.c | 2 +-
net/tap.c | 4 ++--
net/vhost-user.c | 2 +-
numa.c | 4 ++--
qemu-char.c | 24 ++++++++++++------------
scripts/qapi-types.py | 2 +-
scripts/qapi-visit.py | 7 ++-----
scripts/qapi.py | 2 +-
tests/test-qmp-commands.c | 2 +-
tests/test-qmp-input-visitor.c | 10 +++++-----
tests/test-qmp-output-visitor.c | 8 ++++----
tpm.c | 2 +-
ui/input-keymap.c | 10 +++++-----
ui/input-legacy.c | 2 +-
ui/input.c | 22 +++++++++++-----------
util/qemu-sockets.c | 12 ++++++------
29 files changed, 90 insertions(+), 93 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 76c331b..9e1fcc5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2535,7 +2535,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
*spec_info = (ImageInfoSpecific){
- .kind = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
+ .type = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
{
.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
},
diff --git a/block/vmdk.c b/block/vmdk.c
index fbaab67..180f416 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2153,7 +2153,7 @@ static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
ImageInfoList **next;
*spec_info = (ImageInfoSpecific){
- .kind = IMAGE_INFO_SPECIFIC_KIND_VMDK,
+ .type = IMAGE_INFO_SPECIFIC_KIND_VMDK,
{
.vmdk = g_new0(ImageInfoSpecificVmdk, 1),
},
diff --git a/blockdev.c b/blockdev.c
index 4125ff6..fcaa63a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1052,12 +1052,12 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
}
}
-static void blockdev_do_action(int kind, void *data, Error **errp)
+static void blockdev_do_action(int type, void *data, Error **errp)
{
TransactionAction action;
TransactionActionList list;
- action.kind = kind;
+ action.type = type;
action.data = data;
list.value = &action;
list.next = NULL;
@@ -1297,7 +1297,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
InternalSnapshotState *state;
int ret1;
- g_assert(common->action->kind ==
+ g_assert(common->action->type ==
TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
internal = common->action->blockdev_snapshot_internal_sync;
state = DO_UPCAST(InternalSnapshotState, common, common);
@@ -1440,7 +1440,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
TransactionAction *action = common->action;
/* get parameters */
- g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
+ g_assert(action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
has_device = action->blockdev_snapshot_sync->has_device;
device = action->blockdev_snapshot_sync->device;
@@ -1585,7 +1585,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
DriveBackup *backup;
Error *local_err = NULL;
- assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
+ assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
backup = common->action->drive_backup;
blk = blk_by_name(backup->device);
@@ -1653,7 +1653,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
BlockBackend *blk;
Error *local_err = NULL;
- assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
+ assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
backup = common->action->blockdev_backup;
blk = blk_by_name(backup->device);
@@ -1780,9 +1780,9 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
dev_info = dev_entry->value;
dev_entry = dev_entry->next;
- assert(dev_info->kind < ARRAY_SIZE(actions));
+ assert(dev_info->type < ARRAY_SIZE(actions));
- ops = &actions[dev_info->kind];
+ ops = &actions[dev_info->type];
assert(ops->instance_size > 0);
state = g_malloc0(ops->instance_size);
diff --git a/hmp.c b/hmp.c
index dcc66f1..f8fb72f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -830,9 +830,9 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
c, TpmModel_lookup[ti->model]);
monitor_printf(mon, " \\ %s: type=%s",
- ti->id, TpmTypeOptionsKind_lookup[ti->options->kind]);
+ ti->id, TpmTypeOptionsKind_lookup[ti->options->type]);
- switch (ti->options->kind) {
+ switch (ti->options->type) {
case TPM_TYPE_OPTIONS_KIND_PASSTHROUGH:
tpo = ti->options->passthrough;
monitor_printf(mon, "%s%s%s%s",
@@ -1714,14 +1714,14 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
if (*endp != '\0') {
goto err_out;
}
- keylist->value->kind = KEY_VALUE_KIND_NUMBER;
+ keylist->value->type = KEY_VALUE_KIND_NUMBER;
keylist->value->number = value;
} else {
int idx = index_from_key(keyname_buf);
if (idx == Q_KEY_CODE_MAX) {
goto err_out;
}
- keylist->value->kind = KEY_VALUE_KIND_QCODE;
+ keylist->value->type = KEY_VALUE_KIND_QCODE;
keylist->value->qcode = idx;
}
@@ -1937,12 +1937,12 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
value = info->value;
if (value) {
- switch (value->kind) {
+ switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
di = value->dimm;
monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
- MemoryDeviceInfoKind_lookup[value->kind],
+ MemoryDeviceInfoKind_lookup[value->type],
di->id ? di->id : "");
monitor_printf(mon, " addr: 0x%" PRIx64 "\n", di->addr);
monitor_printf(mon, " slot: %" PRId64 "\n", di->slot);
diff --git a/hw/input/hid.c b/hw/input/hid.c
index 21ebd9e..ac02f88 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -119,7 +119,7 @@ static void hid_pointer_event(DeviceState *dev, QemuConsole *src,
assert(hs->n < QUEUE_LENGTH);
e = &hs->ptr.queue[(hs->head + hs->n) & QUEUE_MASK];
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_REL:
if (evt->rel->axis == INPUT_AXIS_X) {
e->xdx += evt->rel->value;
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index fdbe565..58decf2 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -393,7 +393,7 @@ static void ps2_mouse_event(DeviceState *dev, QemuConsole *src,
if (!(s->mouse_status & MOUSE_STATUS_ENABLED))
return;
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_REL:
if (evt->rel->axis == INPUT_AXIS_X) {
s->mouse_dx += evt->rel->value;
diff --git a/hw/input/virtio-input-hid.c b/hw/input/virtio-input-hid.c
index 4d85dad..362dad3 100644
--- a/hw/input/virtio-input-hid.c
+++ b/hw/input/virtio-input-hid.c
@@ -191,7 +191,7 @@ static void virtio_input_handle_event(DeviceState *dev, QemuConsole *src,
virtio_input_event event;
int qcode;
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_KEY:
qcode = qemu_input_key_value_to_qcode(evt->key->key);
if (qcode && keymap_qcode[qcode]) {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index bb04862..a444195 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -196,7 +196,7 @@ ram_addr_t get_current_ram_size(void)
MemoryDeviceInfo *value = info->value;
if (value) {
- switch (value->kind) {
+ switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
size += value->dimm->size;
break;
diff --git a/net/dump.c b/net/dump.c
index 02c8064..f4319cd 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -154,7 +154,7 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
char def_file[128];
const NetdevDumpOptions *dump;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_DUMP);
dump = opts->dump;
assert(peer);
diff --git a/net/hub.c b/net/hub.c
index 3047f12..2dce788 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -285,7 +285,7 @@ int net_init_hubport(const NetClientOptions *opts, const char *name,
{
const NetdevHubPortOptions *hubport;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_HUBPORT);
assert(!peer);
hubport = opts->hubport;
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 4f9bcee..7c6c57f 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -545,7 +545,7 @@ int net_init_l2tpv3(const NetClientOptions *opts,
s->queue_tail = 0;
s->header_mismatch = false;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_L2TPV3);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_L2TPV3);
l2tpv3 = opts->l2tpv3;
if (l2tpv3->has_ipv6 && l2tpv3->ipv6) {
diff --git a/net/net.c b/net/net.c
index 28a5597..36da3f4 100644
--- a/net/net.c
+++ b/net/net.c
@@ -820,7 +820,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
NICInfo *nd;
const NetLegacyNicOptions *nic;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_NIC);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_NIC);
nic = opts->nic;
idx = nic_get_free_idx();
@@ -922,9 +922,9 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
opts = netdev->opts;
name = netdev->id;
- if (opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP ||
- opts->kind == NET_CLIENT_OPTIONS_KIND_NIC ||
- !net_client_init_fun[opts->kind]) {
+ if (opts->type == NET_CLIENT_OPTIONS_KIND_DUMP ||
+ opts->type == NET_CLIENT_OPTIONS_KIND_NIC ||
+ !net_client_init_fun[opts->type]) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a netdev backend type");
return -1;
@@ -935,16 +935,16 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
/* missing optional values have been initialized to "all bits zero" */
name = net->has_id ? net->id : net->name;
- if (opts->kind == NET_CLIENT_OPTIONS_KIND_NONE) {
+ if (opts->type == NET_CLIENT_OPTIONS_KIND_NONE) {
return 0; /* nothing to do */
}
- if (opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
+ if (opts->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a net type");
return -1;
}
- if (!net_client_init_fun[opts->kind]) {
+ if (!net_client_init_fun[opts->type]) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a net backend type (maybe it is not compiled "
"into this binary)");
@@ -952,17 +952,17 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
}
/* Do not add to a vlan if it's a nic with a netdev= parameter. */
- if (opts->kind != NET_CLIENT_OPTIONS_KIND_NIC ||
+ if (opts->type != NET_CLIENT_OPTIONS_KIND_NIC ||
!opts->nic->has_netdev) {
peer = net_hub_add_port(net->has_vlan ? net->vlan : 0, NULL);
}
}
- if (net_client_init_fun[opts->kind](opts, name, peer, errp) < 0) {
+ if (net_client_init_fun[opts->type](opts, name, peer, errp) < 0) {
/* FIXME drop when all init functions store an Error */
if (errp && !*errp) {
error_setg(errp, QERR_DEVICE_INIT_FAILED,
- NetClientOptionsKind_lookup[opts->kind]);
+ NetClientOptionsKind_lookup[opts->type]);
}
return -1;
}
diff --git a/net/slirp.c b/net/slirp.c
index 7657b38..cd06a30 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -746,7 +746,7 @@ int net_init_slirp(const NetClientOptions *opts, const char *name,
const NetdevUserOptions *user;
const char **dnssearch;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_USER);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_USER);
user = opts->user;
vnet = user->has_net ? g_strdup(user->net) :
diff --git a/net/socket.c b/net/socket.c
index b1e3b1c..faf2823 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -706,7 +706,7 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
Error *err = NULL;
const NetdevSocketOptions *sock;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_SOCKET);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_SOCKET);
sock = opts->socket;
if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
diff --git a/net/tap.c b/net/tap.c
index bd01590..3cf540b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -565,7 +565,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
TAPState *s;
int fd, vnet_hdr;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_BRIDGE);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_BRIDGE);
bridge = opts->bridge;
helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
@@ -728,7 +728,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
const char *vhostfdname;
char ifname[128];
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_TAP);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_TAP);
tap = opts->tap;
queues = tap->has_queues ? tap->queues : 1;
vhostfdname = tap->has_vhostfd ? tap->vhostfd : NULL;
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 93dcecd..16393c6 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -229,7 +229,7 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
const NetdevVhostUserOptions *vhost_user_opts;
CharDriverState *chr;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
vhost_user_opts = opts->vhost_user;
chr = net_vhost_parse_chardev(vhost_user_opts, errp);
diff --git a/numa.c b/numa.c
index 402804b..aa9e0f7 100644
--- a/numa.c
+++ b/numa.c
@@ -226,7 +226,7 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
goto error;
}
- switch (object->kind) {
+ switch (object->type) {
case NUMA_OPTIONS_KIND_NODE:
numa_node_parse(object->node, opts, &err);
if (err) {
@@ -487,7 +487,7 @@ static void numa_stat_memory_devices(uint64_t node_mem[])
MemoryDeviceInfo *value = info->value;
if (value) {
- switch (value->kind) {
+ switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
node_mem[value->dimm->node] += value->dimm->size;
break;
diff --git a/qemu-char.c b/qemu-char.c
index d956f8d..20fc51f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -123,7 +123,7 @@ static int SocketAddress_to_str(char *dest, int max_len,
const char *prefix, SocketAddress *addr,
bool is_listen, bool is_telnet)
{
- switch (addr->kind) {
+ switch (addr->type) {
case SOCKET_ADDRESS_KIND_INET:
return snprintf(dest, max_len, "%s%s:%s:%s%s", prefix,
is_telnet ? "telnet" : "tcp", addr->inet->host,
@@ -3570,11 +3570,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
addr = g_new0(SocketAddress, 1);
if (path) {
- addr->kind = SOCKET_ADDRESS_KIND_UNIX;
+ addr->type = SOCKET_ADDRESS_KIND_UNIX;
addr->q_unix = g_new0(UnixSocketAddress, 1);
addr->q_unix->path = g_strdup(path);
} else {
- addr->kind = SOCKET_ADDRESS_KIND_INET;
+ addr->type = SOCKET_ADDRESS_KIND_INET;
addr->inet = g_new0(InetSocketAddress, 1);
addr->inet->host = g_strdup(host);
addr->inet->port = g_strdup(port);
@@ -3619,7 +3619,7 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
backend->udp = g_new0(ChardevUdp, 1);
addr = g_new0(SocketAddress, 1);
- addr->kind = SOCKET_ADDRESS_KIND_INET;
+ addr->type = SOCKET_ADDRESS_KIND_INET;
addr->inet = g_new0(InetSocketAddress, 1);
addr->inet->host = g_strdup(host);
addr->inet->port = g_strdup(port);
@@ -3632,7 +3632,7 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
if (has_local) {
backend->udp->has_local = true;
addr = g_new0(SocketAddress, 1);
- addr->kind = SOCKET_ADDRESS_KIND_INET;
+ addr->type = SOCKET_ADDRESS_KIND_INET;
addr->inet = g_new0(InetSocketAddress, 1);
addr->inet->host = g_strdup(localaddr);
addr->inet->port = g_strdup(localport);
@@ -3704,7 +3704,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
}
chr = NULL;
- backend->kind = cd->kind;
+ backend->type = cd->kind;
if (cd->parse) {
cd->parse(opts, backend, &local_err);
if (local_err) {
@@ -3722,7 +3722,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
qapi_free_ChardevReturn(ret);
backend = g_new0(ChardevBackend, 1);
backend->mux = g_new0(ChardevMux, 1);
- backend->kind = CHARDEV_BACKEND_KIND_MUX;
+ backend->type = CHARDEV_BACKEND_KIND_MUX;
backend->mux->chardev = g_strdup(bid);
ret = qmp_chardev_add(id, backend, errp);
if (!ret) {
@@ -4151,7 +4151,7 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
s->fd = -1;
s->listen_fd = -1;
- s->is_unix = addr->kind == SOCKET_ADDRESS_KIND_UNIX;
+ s->is_unix = addr->type == SOCKET_ADDRESS_KIND_UNIX;
s->is_listen = is_listen;
s->is_telnet = is_telnet;
s->do_nodelay = do_nodelay;
@@ -4225,7 +4225,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
return NULL;
}
- switch (backend->kind) {
+ switch (backend->type) {
case CHARDEV_BACKEND_KIND_FILE:
chr = qmp_chardev_open_file(backend->file, errp);
break;
@@ -4296,7 +4296,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
chr = qemu_chr_open_ringbuf(backend->ringbuf, errp);
break;
default:
- error_setg(errp, "unknown chardev backend (%d)", backend->kind);
+ error_setg(errp, "unknown chardev backend (%d)", backend->type);
break;
}
@@ -4312,9 +4312,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
if (chr) {
chr->label = g_strdup(id);
chr->avail_connections =
- (backend->kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
+ (backend->type == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
if (!chr->filename) {
- chr->filename = g_strdup(ChardevBackendKind_lookup[backend->kind]);
+ chr->filename = g_strdup(ChardevBackendKind_lookup[backend->type]);
}
if (!chr->explicit_be_open) {
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9caf7bd..2d09b90 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -127,7 +127,7 @@ struct %(c_name)s {
''')
else:
ret += mcgen('''
- %(c_type)s kind;
+ %(c_type)s type;
''',
c_type=c_name(variants.tag_member.type.name))
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 6705eaf..eb96e36 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -200,11 +200,11 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(m, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
+ visit_get_next_type(m, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err);
if (err) {
goto out_end;
}
- switch ((*obj)->kind) {
+ switch ((*obj)->type) {
''',
c_name=c_name(name))
@@ -270,9 +270,6 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
c_name=c_name(name))
tag_key = variants.tag_member.name
- if not variants.tag_name:
- # we pointlessly use a different key for simple unions
- tag_key = 'type'
ret += mcgen('''
visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
if (err) {
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 34c2884..99ea61d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -933,7 +933,7 @@ class QAPISchemaObjectTypeVariants(object):
assert not tag_enum
self.tag_member = None
else:
- self.tag_member = QAPISchemaObjectTypeMember('kind', tag_enum,
+ self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
False)
self.variants = variants
def check(self, schema, members, seen):
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 8d5249e..6a67e73 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -64,7 +64,7 @@ __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
{
__org_qemu_x_Union1 *ret = g_new0(__org_qemu_x_Union1, 1);
- ret->kind = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
+ ret->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
ret->__org_qemu_x_branch = strdup("blah1");
return ret;
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 61715b3..da84993 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -374,7 +374,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
visit_type_UserDefAlternate(v, &tmp, NULL, &err);
g_assert(err == NULL);
- g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
+ g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
g_assert_cmpint(tmp->i, ==, 42);
qapi_free_UserDefAlternate(tmp);
}
@@ -404,7 +404,7 @@ static void test_native_list_integer_helper(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, kind);
+ g_assert_cmpint(cvalue->type, ==, kind);
switch (kind) {
case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
@@ -567,7 +567,7 @@ static void test_visitor_in_native_list_bool(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN);
+ g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN);
for (i = 0, elem = cvalue->boolean; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, (i % 3 == 0) ? 1 : 0);
@@ -602,7 +602,7 @@ static void test_visitor_in_native_list_string(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_STRING);
+ g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_STRING);
for (i = 0, elem = cvalue->string; elem; elem = elem->next, i++) {
gchar str[8];
@@ -641,7 +641,7 @@ static void test_visitor_in_native_list_number(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER);
+ g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER);
for (i = 0, elem = cvalue->number; elem; elem = elem->next, i++) {
GString *double_expected = g_string_new("");
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 1a28dc2..2162a4a 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -517,7 +517,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
Error *err = NULL;
UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate));
- tmp->kind = USER_DEF_ALTERNATE_KIND_I;
+ tmp->type = USER_DEF_ALTERNATE_KIND_I;
tmp->i = 42;
visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err);
@@ -542,7 +542,7 @@ static void test_visitor_out_empty(TestOutputVisitorData *data,
static void init_native_list(UserDefNativeListUnion *cvalue)
{
int i;
- switch (cvalue->kind) {
+ switch (cvalue->type) {
case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
intList **list = &cvalue->integer;
for (i = 0; i < 32; i++) {
@@ -763,14 +763,14 @@ static void test_native_list(TestOutputVisitorData *data,
Error *err = NULL;
QObject *obj;
- cvalue->kind = kind;
+ cvalue->type = kind;
init_native_list(cvalue);
visit_type_UserDefNativeListUnion(data->ov, &cvalue, NULL, &err);
g_assert(err == NULL);
obj = qmp_output_get_qobject(data->qov);
- check_native_list(obj, cvalue->kind);
+ check_native_list(obj, cvalue->type);
qapi_free_UserDefNativeListUnion(cvalue);
qobject_decref(obj);
}
diff --git a/tpm.c b/tpm.c
index 4e9b109..fcab81c 100644
--- a/tpm.c
+++ b/tpm.c
@@ -260,7 +260,7 @@ static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)
switch (drv->ops->type) {
case TPM_TYPE_PASSTHROUGH:
- res->options->kind = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;
+ res->options->type = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;
tpo = g_new0(TPMPassthroughOptions, 1);
res->options->passthrough = tpo;
if (drv->path) {
diff --git a/ui/input-keymap.c b/ui/input-keymap.c
index 7635cb0..088523d 100644
--- a/ui/input-keymap.c
+++ b/ui/input-keymap.c
@@ -139,10 +139,10 @@ static int number_to_qcode[0x100];
int qemu_input_key_value_to_number(const KeyValue *value)
{
- if (value->kind == KEY_VALUE_KIND_QCODE) {
+ if (value->type == KEY_VALUE_KIND_QCODE) {
return qcode_to_number[value->qcode];
} else {
- assert(value->kind == KEY_VALUE_KIND_NUMBER);
+ assert(value->type == KEY_VALUE_KIND_NUMBER);
return value->number;
}
}
@@ -166,10 +166,10 @@ int qemu_input_key_number_to_qcode(uint8_t nr)
int qemu_input_key_value_to_qcode(const KeyValue *value)
{
- if (value->kind == KEY_VALUE_KIND_QCODE) {
+ if (value->type == KEY_VALUE_KIND_QCODE) {
return value->qcode;
} else {
- assert(value->kind == KEY_VALUE_KIND_NUMBER);
+ assert(value->type == KEY_VALUE_KIND_NUMBER);
return qemu_input_key_number_to_qcode(value->number);
}
}
@@ -180,7 +180,7 @@ int qemu_input_key_value_to_scancode(const KeyValue *value, bool down,
int keycode = qemu_input_key_value_to_number(value);
int count = 0;
- if (value->kind == KEY_VALUE_KIND_QCODE &&
+ if (value->type == KEY_VALUE_KIND_QCODE &&
value->qcode == Q_KEY_CODE_PAUSE) {
/* specific case */
int v = down ? 0 : 0x80;
diff --git a/ui/input-legacy.c b/ui/input-legacy.c
index e50f296..6149648 100644
--- a/ui/input-legacy.c
+++ b/ui/input-legacy.c
@@ -150,7 +150,7 @@ static void legacy_mouse_event(DeviceState *dev, QemuConsole *src,
};
QEMUPutMouseEntry *s = (QEMUPutMouseEntry *)dev;
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_BTN:
if (evt->btn->down) {
s->buttons |= bmap[evt->btn->button];
diff --git a/ui/input.c b/ui/input.c
index 1a552d1..fd86571 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -147,10 +147,10 @@ void qmp_x_input_send_event(bool has_console, int64_t console,
for (e = events; e != NULL; e = e->next) {
InputEvent *event = e->value;
- if (!qemu_input_find_handler(1 << event->kind, con)) {
+ if (!qemu_input_find_handler(1 << event->type, con)) {
error_setg(errp, "Input handler not found for "
"event type %s",
- InputEventKind_lookup[event->kind]);
+ InputEventKind_lookup[event->type]);
return;
}
}
@@ -197,9 +197,9 @@ static void qemu_input_event_trace(QemuConsole *src, InputEvent *evt)
if (src) {
idx = qemu_console_get_index(src);
}
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_KEY:
- switch (evt->key->key->kind) {
+ switch (evt->key->key->type) {
case KEY_VALUE_KIND_NUMBER:
qcode = qemu_input_key_number_to_qcode(evt->key->key->number);
name = QKeyCode_lookup[qcode];
@@ -311,12 +311,12 @@ void qemu_input_event_send(QemuConsole *src, InputEvent *evt)
qemu_input_event_trace(src, evt);
/* pre processing */
- if (graphic_rotate && (evt->kind == INPUT_EVENT_KIND_ABS)) {
+ if (graphic_rotate && (evt->type == INPUT_EVENT_KIND_ABS)) {
qemu_input_transform_abs_rotate(evt);
}
/* send event */
- s = qemu_input_find_handler(1 << evt->kind, src);
+ s = qemu_input_find_handler(1 << evt->type, src);
if (!s) {
return;
}
@@ -349,7 +349,7 @@ InputEvent *qemu_input_event_new_key(KeyValue *key, bool down)
{
InputEvent *evt = g_new0(InputEvent, 1);
evt->key = g_new0(InputKeyEvent, 1);
- evt->kind = INPUT_EVENT_KIND_KEY;
+ evt->type = INPUT_EVENT_KIND_KEY;
evt->key->key = key;
evt->key->down = down;
return evt;
@@ -372,7 +372,7 @@ void qemu_input_event_send_key(QemuConsole *src, KeyValue *key, bool down)
void qemu_input_event_send_key_number(QemuConsole *src, int num, bool down)
{
KeyValue *key = g_new0(KeyValue, 1);
- key->kind = KEY_VALUE_KIND_NUMBER;
+ key->type = KEY_VALUE_KIND_NUMBER;
key->number = num;
qemu_input_event_send_key(src, key, down);
}
@@ -380,7 +380,7 @@ void qemu_input_event_send_key_number(QemuConsole *src, int num, bool down)
void qemu_input_event_send_key_qcode(QemuConsole *src, QKeyCode q, bool down)
{
KeyValue *key = g_new0(KeyValue, 1);
- key->kind = KEY_VALUE_KIND_QCODE;
+ key->type = KEY_VALUE_KIND_QCODE;
key->qcode = q;
qemu_input_event_send_key(src, key, down);
}
@@ -399,7 +399,7 @@ InputEvent *qemu_input_event_new_btn(InputButton btn, bool down)
{
InputEvent *evt = g_new0(InputEvent, 1);
evt->btn = g_new0(InputBtnEvent, 1);
- evt->kind = INPUT_EVENT_KIND_BTN;
+ evt->type = INPUT_EVENT_KIND_BTN;
evt->btn->button = btn;
evt->btn->down = down;
return evt;
@@ -451,7 +451,7 @@ InputEvent *qemu_input_event_new_move(InputEventKind kind,
InputEvent *evt = g_new0(InputEvent, 1);
InputMoveEvent *move = g_new0(InputMoveEvent, 1);
- evt->kind = kind;
+ evt->type = kind;
evt->data = move;
move->axis = axis;
move->value = value;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 2add83a..277b139 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -904,7 +904,7 @@ SocketAddress *socket_parse(const char *str, Error **errp)
error_setg(errp, "invalid Unix socket address");
goto fail;
} else {
- addr->kind = SOCKET_ADDRESS_KIND_UNIX;
+ addr->type = SOCKET_ADDRESS_KIND_UNIX;
addr->q_unix = g_new(UnixSocketAddress, 1);
addr->q_unix->path = g_strdup(str + 5);
}
@@ -913,12 +913,12 @@ SocketAddress *socket_parse(const char *str, Error **errp)
error_setg(errp, "invalid file descriptor address");
goto fail;
} else {
- addr->kind = SOCKET_ADDRESS_KIND_FD;
+ addr->type = SOCKET_ADDRESS_KIND_FD;
addr->fd = g_new(String, 1);
addr->fd->str = g_strdup(str + 3);
}
} else {
- addr->kind = SOCKET_ADDRESS_KIND_INET;
+ addr->type = SOCKET_ADDRESS_KIND_INET;
addr->inet = inet_parse(str, errp);
if (addr->inet == NULL) {
goto fail;
@@ -938,7 +938,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
int fd;
opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
- switch (addr->kind) {
+ switch (addr->type) {
case SOCKET_ADDRESS_KIND_INET:
inet_addr_to_opts(opts, addr->inet);
fd = inet_connect_opts(opts, errp, callback, opaque);
@@ -970,7 +970,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
int fd;
opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
- switch (addr->kind) {
+ switch (addr->type) {
case SOCKET_ADDRESS_KIND_INET:
inet_addr_to_opts(opts, addr->inet);
fd = inet_listen_opts(opts, 0, errp);
@@ -998,7 +998,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
int fd;
opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
- switch (remote->kind) {
+ switch (remote->type) {
case SOCKET_ADDRESS_KIND_INET:
inet_addr_to_opts(opts, remote->inet);
if (local) {
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 02/20] vnc: hoist allocation of VncBasicInfo to callers
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 01/20] qapi: use 'type' in generated C code to match QMP union wire form Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 03/20] qapi: Unbox base members Eric Blake
` (17 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Gerd Hoffmann
A future qapi patch will rework generated structs with a base
class to be unboxed. In preparation for that, change the code
that allocates then populates an info struct to instead merely
populate the fields of an info field passed in as a parameter.
Add rudimentary Error handling for cases where the old code
returned NULL; but as before, callers merely ignore errors for
now.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
ui/vnc.c | 52 ++++++++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index e26973a..273161c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -153,10 +153,11 @@ char *vnc_socket_remote_addr(const char *format, int fd) {
return addr_to_string(format, &sa, salen);
}
-static VncBasicInfo *vnc_basic_info_get(struct sockaddr_storage *sa,
- socklen_t salen)
+static void vnc_basic_info_get(struct sockaddr_storage *sa,
+ socklen_t salen,
+ VncBasicInfo *info,
+ Error **errp)
{
- VncBasicInfo *info;
char host[NI_MAXHOST];
char serv[NI_MAXSERV];
int err;
@@ -165,42 +166,44 @@ static VncBasicInfo *vnc_basic_info_get(struct sockaddr_storage *sa,
host, sizeof(host),
serv, sizeof(serv),
NI_NUMERICHOST | NI_NUMERICSERV)) != 0) {
- VNC_DEBUG("Cannot resolve address %d: %s\n",
- err, gai_strerror(err));
- return NULL;
+ error_setg(errp, "Cannot resolve address %d: %s",
+ err, gai_strerror(err));
+ return;
}
- info = g_malloc0(sizeof(VncBasicInfo));
info->host = g_strdup(host);
info->service = g_strdup(serv);
info->family = inet_netfamily(sa->ss_family);
- return info;
}
-static VncBasicInfo *vnc_basic_info_get_from_server_addr(int fd)
+static void vnc_basic_info_get_from_server_addr(int fd, VncBasicInfo *info,
+ Error **errp)
{
struct sockaddr_storage sa;
socklen_t salen;
salen = sizeof(sa);
if (getsockname(fd, (struct sockaddr*)&sa, &salen) < 0) {
- return NULL;
+ error_setg_errno(errp, errno, "getsockname failed");
+ return;
}
- return vnc_basic_info_get(&sa, salen);
+ vnc_basic_info_get(&sa, salen, info, errp);
}
-static VncBasicInfo *vnc_basic_info_get_from_remote_addr(int fd)
+static void vnc_basic_info_get_from_remote_addr(int fd, VncBasicInfo *info,
+ Error **errp)
{
struct sockaddr_storage sa;
socklen_t salen;
salen = sizeof(sa);
if (getpeername(fd, (struct sockaddr*)&sa, &salen) < 0) {
- return NULL;
+ error_setg_errno(errp, errno, "getpeername failed");
+ return;
}
- return vnc_basic_info_get(&sa, salen);
+ vnc_basic_info_get(&sa, salen, info, errp);
}
static const char *vnc_auth_name(VncDisplay *vd) {
@@ -257,13 +260,10 @@ static const char *vnc_auth_name(VncDisplay *vd) {
static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
{
VncServerInfo *info;
- VncBasicInfo *bi = vnc_basic_info_get_from_server_addr(vd->lsock);
- if (!bi) {
- return NULL;
- }
info = g_malloc(sizeof(*info));
- info->base = bi;
+ info->base = g_malloc(sizeof(*info->base));
+ vnc_basic_info_get_from_server_addr(vd->lsock, info->base, NULL);
info->has_auth = true;
info->auth = g_strdup(vnc_auth_name(vd));
return info;
@@ -293,11 +293,15 @@ static void vnc_client_cache_auth(VncState *client)
static void vnc_client_cache_addr(VncState *client)
{
- VncBasicInfo *bi = vnc_basic_info_get_from_remote_addr(client->csock);
-
- if (bi) {
- client->info = g_malloc0(sizeof(*client->info));
- client->info->base = bi;
+ Error *err = NULL;
+ client->info = g_malloc0(sizeof(*client->info));
+ client->info->base = g_malloc0(sizeof(*client->info->base));
+ vnc_basic_info_get_from_remote_addr(client->csock, client->info->base,
+ &err);
+ if (err) {
+ qapi_free_VncClientInfo(client->info);
+ client->info = NULL;
+ error_free(err);
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 03/20] qapi: Unbox base members
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 01/20] qapi: use 'type' in generated C code to match QMP union wire form Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 02/20] vnc: hoist allocation of VncBasicInfo to callers Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 04/20] qapi-visit: Remove redundant functions for flat union base Eric Blake
` (16 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Gerd Hoffmann, armbru, Luiz Capitulino
Rather than storing a base class as a pointer to a box, just
store the fields of that base class in the same order, so that
a child struct can be safely cast to its parent. This gives
less malloc overhead, less pointer dereferencing, and even less
generated code.
Without boxing, the corner case of one empty struct having
another empty struct as its base type now requires inserting a
dummy member (previously, the pointer to the base would have
sufficed).
Compare to the earlier commit "qapi: Generate a nicer struct for
flat unions".
Signed-off-by: Eric Blake <eblake@redhat.com>
---
hmp.c | 6 +++---
scripts/qapi-types.py | 29 +++++++++++++++--------------
scripts/qapi-visit.py | 9 ++++++---
tests/test-qmp-commands.c | 15 +++++----------
tests/test-qmp-event.c | 8 ++------
tests/test-qmp-input-visitor.c | 4 ++--
tests/test-qmp-output-visitor.c | 12 ++++--------
tests/test-visitor-serialization.c | 14 ++++++--------
ui/spice-core.c | 23 +++++++++++++----------
ui/vnc.c | 20 +++++++++-----------
10 files changed, 65 insertions(+), 75 deletions(-)
diff --git a/hmp.c b/hmp.c
index f8fb72f..9f202df 100644
--- a/hmp.c
+++ b/hmp.c
@@ -558,8 +558,8 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
for (client = info->clients; client; client = client->next) {
monitor_printf(mon, "Client:\n");
monitor_printf(mon, " address: %s:%s\n",
- client->value->base->host,
- client->value->base->service);
+ client->value->host,
+ client->value->service);
monitor_printf(mon, " x509_dname: %s\n",
client->value->x509_dname ?
client->value->x509_dname : "none");
@@ -627,7 +627,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
for (chan = info->channels; chan; chan = chan->next) {
monitor_printf(mon, "Channel:\n");
monitor_printf(mon, " address: %s:%s%s\n",
- chan->value->base->host, chan->value->base->port,
+ chan->value->host, chan->value->port,
chan->value->tls ? " [tls]" : "");
monitor_printf(mon, " session: %" PRId64 "\n",
chan->value->connection_id);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2d09b90..27ad0c1 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -47,8 +47,19 @@ def gen_struct_field(name, typ, optional):
c_type=typ.c_type(), c_name=c_name(name))
return ret
-def gen_struct_fields(members):
+def gen_struct_fields(members, base, nested=False):
ret = ''
+ if base:
+ if not nested:
+ ret += mcgen('''
+ /* Members inherited from %(c_name)s: */
+''',
+ c_name=base.c_name())
+ ret += gen_struct_fields(base.local_members, base.base, True)
+ if not nested:
+ ret += mcgen('''
+ /* Own members: */
+''')
for memb in members:
ret += gen_struct_field(memb.name, memb.type, memb.optional)
@@ -61,15 +72,12 @@ struct %(c_name)s {
''',
c_name=c_name(name))
- if base:
- ret += gen_struct_field('base', base, False)
-
- ret += gen_struct_fields(members)
+ ret += gen_struct_fields(members, base)
# Make sure that all structs have at least one field; this avoids
# potential issues with attempting to malloc space for zero-length structs
# in C, and also incompatibility with C++ (where an empty struct is size 1).
- if not base and not members:
+ if not (base and base.members) and not members:
ret += mcgen('''
char qapi_dummy_field_for_empty_struct;
''')
@@ -117,14 +125,7 @@ struct %(c_name)s {
''',
c_name=c_name(name))
if base:
- ret += mcgen('''
- /* Members inherited from %(c_name)s: */
-''',
- c_name=c_name(base.name))
- ret += gen_struct_fields(base.members)
- ret += mcgen('''
- /* Own members: */
-''')
+ ret += gen_struct_fields([], base)
else:
ret += mcgen('''
%(c_type)s type;
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index eb96e36..ef96345 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -59,12 +59,15 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
return ret
def gen_visit_struct_fields(name, base, members):
+ if name in struct_fields_seen:
+ return ''
struct_fields_seen.add(name)
ret = ''
if base:
- ret += gen_visit_implicit_struct(base)
+ ret += gen_visit_struct_fields(base.name, base.base,
+ base.local_members)
ret += mcgen('''
@@ -78,12 +81,12 @@ static void visit_type_%(c_name)s_fields(Visitor *m, %(c_name)s **obj, Error **e
if base:
ret += mcgen('''
-visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
+visit_type_%(c_type)s_fields(m, (%(c_type)s **)obj, &err);
if (err) {
goto out;
}
''',
- c_type=base.c_name(), c_name=c_name('base'))
+ c_type=base.c_name())
for memb in members:
if memb.optional:
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 6a67e73..5181823 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -25,11 +25,9 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
UserDefOne *ud1d = g_malloc0(sizeof(UserDefOne));
ud1c->string = strdup(ud1a->string);
- ud1c->base = g_new0(UserDefZero, 1);
- ud1c->base->integer = ud1a->base->integer;
+ ud1c->integer = ud1a->integer;
ud1d->string = strdup(has_udb1 ? ud1b->string : "blah0");
- ud1d->base = g_new0(UserDefZero, 1);
- ud1d->base->integer = has_udb1 ? ud1b->base->integer : 0;
+ ud1d->integer = has_udb1 ? ud1b->integer : 0;
ret = g_new0(UserDefTwo, 1);
ret->string0 = strdup("blah1");
@@ -176,20 +174,17 @@ static void test_dealloc_types(void)
UserDefOneList *ud1list;
ud1test = g_malloc0(sizeof(UserDefOne));
- ud1test->base = g_new0(UserDefZero, 1);
- ud1test->base->integer = 42;
+ ud1test->integer = 42;
ud1test->string = g_strdup("hi there 42");
qapi_free_UserDefOne(ud1test);
ud1a = g_malloc0(sizeof(UserDefOne));
- ud1a->base = g_new0(UserDefZero, 1);
- ud1a->base->integer = 43;
+ ud1a->integer = 43;
ud1a->string = g_strdup("hi there 43");
ud1b = g_malloc0(sizeof(UserDefOne));
- ud1b->base = g_new0(UserDefZero, 1);
- ud1b->base->integer = 44;
+ ud1b->integer = 44;
ud1b->string = g_strdup("hi there 44");
ud1list = g_malloc0(sizeof(UserDefOneList));
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 28f146d..035c65c 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -179,9 +179,7 @@ static void test_event_c(TestEventData *data,
QDict *d, *d_data, *d_b;
UserDefOne b;
- UserDefZero z;
- z.integer = 2;
- b.base = &z;
+ b.integer = 2;
b.string = g_strdup("test1");
b.has_enum1 = false;
@@ -209,11 +207,9 @@ static void test_event_d(TestEventData *data,
{
UserDefOne struct1;
EventStructOne a;
- UserDefZero z;
QDict *d, *d_data, *d_a, *d_struct1;
- z.integer = 2;
- struct1.base = &z;
+ struct1.integer = 2;
struct1.string = g_strdup("test1");
struct1.has_enum1 = true;
struct1.enum1 = ENUM_ONE_VALUE1;
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index da84993..b6deee3 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -262,7 +262,7 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
check_and_free_str(udp->string0, "string0");
check_and_free_str(udp->dict1->string1, "string1");
- g_assert_cmpint(udp->dict1->dict2->userdef->base->integer, ==, 42);
+ g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42);
check_and_free_str(udp->dict1->dict2->userdef->string, "string");
check_and_free_str(udp->dict1->dict2->string, "string2");
g_assert(udp->dict1->has_dict3 == false);
@@ -292,7 +292,7 @@ static void test_visitor_in_list(TestInputVisitorData *data,
snprintf(string, sizeof(string), "string%d", i);
g_assert_cmpstr(item->value->string, ==, string);
- g_assert_cmpint(item->value->base->integer, ==, 42 + i);
+ g_assert_cmpint(item->value->integer, ==, 42 + i);
}
qapi_free_UserDefOneList(head);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 2162a4a..ab4bc5d 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -250,16 +250,14 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
ud2->dict1->dict2 = g_malloc0(sizeof(*ud2->dict1->dict2));
ud2->dict1->dict2->userdef = g_new0(UserDefOne, 1);
ud2->dict1->dict2->userdef->string = g_strdup(string);
- ud2->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
- ud2->dict1->dict2->userdef->base->integer = value;
+ ud2->dict1->dict2->userdef->integer = value;
ud2->dict1->dict2->string = g_strdup(strings[2]);
ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3));
ud2->dict1->has_dict3 = true;
ud2->dict1->dict3->userdef = g_new0(UserDefOne, 1);
ud2->dict1->dict3->userdef->string = g_strdup(string);
- ud2->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
- ud2->dict1->dict3->userdef->base->integer = value;
+ ud2->dict1->dict3->userdef->integer = value;
ud2->dict1->dict3->string = g_strdup(strings[3]);
visit_type_UserDefTwo(data->ov, &ud2, "unused", &err);
@@ -301,8 +299,7 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
const void *unused)
{
EnumOne bad_values[] = { ENUM_ONE_MAX, -1 };
- UserDefZero b;
- UserDefOne u = { .base = &b }, *pu = &u;
+ UserDefOne u, *pu = &u;
Error *err;
int i;
@@ -416,8 +413,7 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
p->value->dict1->dict2 = g_new0(UserDefTwoDictDict, 1);
p->value->dict1->dict2->userdef = g_new0(UserDefOne, 1);
p->value->dict1->dict2->userdef->string = g_strdup(string);
- p->value->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
- p->value->dict1->dict2->userdef->base->integer = 42;
+ p->value->dict1->dict2->userdef->integer = 42;
p->value->dict1->dict2->string = g_strdup(string);
p->value->dict1->has_dict3 = false;
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index fa86cae..634563b 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -258,15 +258,13 @@ static UserDefTwo *nested_struct_create(void)
udnp->dict1->string1 = strdup("test_string1");
udnp->dict1->dict2 = g_malloc0(sizeof(*udnp->dict1->dict2));
udnp->dict1->dict2->userdef = g_new0(UserDefOne, 1);
- udnp->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
- udnp->dict1->dict2->userdef->base->integer = 42;
+ udnp->dict1->dict2->userdef->integer = 42;
udnp->dict1->dict2->userdef->string = strdup("test_string");
udnp->dict1->dict2->string = strdup("test_string2");
udnp->dict1->dict3 = g_malloc0(sizeof(*udnp->dict1->dict3));
udnp->dict1->has_dict3 = true;
udnp->dict1->dict3->userdef = g_new0(UserDefOne, 1);
- udnp->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
- udnp->dict1->dict3->userdef->base->integer = 43;
+ udnp->dict1->dict3->userdef->integer = 43;
udnp->dict1->dict3->userdef->string = strdup("test_string");
udnp->dict1->dict3->string = strdup("test_string3");
return udnp;
@@ -278,15 +276,15 @@ static void nested_struct_compare(UserDefTwo *udnp1, UserDefTwo *udnp2)
g_assert(udnp2);
g_assert_cmpstr(udnp1->string0, ==, udnp2->string0);
g_assert_cmpstr(udnp1->dict1->string1, ==, udnp2->dict1->string1);
- g_assert_cmpint(udnp1->dict1->dict2->userdef->base->integer, ==,
- udnp2->dict1->dict2->userdef->base->integer);
+ g_assert_cmpint(udnp1->dict1->dict2->userdef->integer, ==,
+ udnp2->dict1->dict2->userdef->integer);
g_assert_cmpstr(udnp1->dict1->dict2->userdef->string, ==,
udnp2->dict1->dict2->userdef->string);
g_assert_cmpstr(udnp1->dict1->dict2->string, ==,
udnp2->dict1->dict2->string);
g_assert(udnp1->dict1->has_dict3 == udnp2->dict1->has_dict3);
- g_assert_cmpint(udnp1->dict1->dict3->userdef->base->integer, ==,
- udnp2->dict1->dict3->userdef->base->integer);
+ g_assert_cmpint(udnp1->dict1->dict3->userdef->integer, ==,
+ udnp2->dict1->dict3->userdef->integer);
g_assert_cmpstr(udnp1->dict1->dict3->userdef->string, ==,
udnp2->dict1->dict3->userdef->string);
g_assert_cmpstr(udnp1->dict1->dict3->string, ==,
diff --git a/ui/spice-core.c b/ui/spice-core.c
index bf4fd07..86f4441 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -200,8 +200,6 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
{
SpiceServerInfo *server = g_malloc0(sizeof(*server));
SpiceChannel *client = g_malloc0(sizeof(*client));
- server->base = g_malloc0(sizeof(*server->base));
- client->base = g_malloc0(sizeof(*client->base));
/*
* Spice server might have called us from spice worker thread
@@ -218,9 +216,11 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
}
if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
- add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
+ add_addr_info((SpiceBasicInfo *)client,
+ (struct sockaddr *)&info->paddr_ext,
info->plen_ext);
- add_addr_info(server->base, (struct sockaddr *)&info->laddr_ext,
+ add_addr_info((SpiceBasicInfo *)server,
+ (struct sockaddr *)&info->laddr_ext,
info->llen_ext);
} else {
error_report("spice: %s, extended address is expected",
@@ -229,7 +229,9 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
switch (event) {
case SPICE_CHANNEL_EVENT_CONNECTED:
- qapi_event_send_spice_connected(server->base, client->base, &error_abort);
+ qapi_event_send_spice_connected((SpiceBasicInfo *)server,
+ (SpiceBasicInfo *)client,
+ &error_abort);
break;
case SPICE_CHANNEL_EVENT_INITIALIZED:
if (auth) {
@@ -242,7 +244,9 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
break;
case SPICE_CHANNEL_EVENT_DISCONNECTED:
channel_list_del(info);
- qapi_event_send_spice_disconnected(server->base, client->base, &error_abort);
+ qapi_event_send_spice_disconnected((SpiceBasicInfo *)server,
+ (SpiceBasicInfo *)client,
+ &error_abort);
break;
default:
break;
@@ -378,16 +382,15 @@ static SpiceChannelList *qmp_query_spice_channels(void)
chan = g_malloc0(sizeof(*chan));
chan->value = g_malloc0(sizeof(*chan->value));
- chan->value->base = g_malloc0(sizeof(*chan->value->base));
paddr = (struct sockaddr *)&item->info->paddr_ext;
plen = item->info->plen_ext;
getnameinfo(paddr, plen,
host, sizeof(host), port, sizeof(port),
NI_NUMERICHOST | NI_NUMERICSERV);
- chan->value->base->host = g_strdup(host);
- chan->value->base->port = g_strdup(port);
- chan->value->base->family = inet_netfamily(paddr->sa_family);
+ chan->value->host = g_strdup(host);
+ chan->value->port = g_strdup(port);
+ chan->value->family = inet_netfamily(paddr->sa_family);
chan->value->connection_id = item->info->connection_id;
chan->value->channel_type = item->info->type;
diff --git a/ui/vnc.c b/ui/vnc.c
index 273161c..ab76039 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -262,8 +262,7 @@ static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
VncServerInfo *info;
info = g_malloc(sizeof(*info));
- info->base = g_malloc(sizeof(*info->base));
- vnc_basic_info_get_from_server_addr(vd->lsock, info->base, NULL);
+ vnc_basic_info_get_from_server_addr(vd->lsock, (VncBasicInfo *)info, NULL);
info->has_auth = true;
info->auth = g_strdup(vnc_auth_name(vd));
return info;
@@ -295,8 +294,8 @@ static void vnc_client_cache_addr(VncState *client)
{
Error *err = NULL;
client->info = g_malloc0(sizeof(*client->info));
- client->info->base = g_malloc0(sizeof(*client->info->base));
- vnc_basic_info_get_from_remote_addr(client->csock, client->info->base,
+ vnc_basic_info_get_from_remote_addr(client->csock,
+ (VncBasicInfo *)client->info,
&err);
if (err) {
qapi_free_VncClientInfo(client->info);
@@ -312,7 +311,6 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
if (!vs->info) {
return;
}
- g_assert(vs->info->base);
si = vnc_server_info_get(vs->vd);
if (!si) {
@@ -321,7 +319,8 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
switch (event) {
case QAPI_EVENT_VNC_CONNECTED:
- qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
+ qapi_event_send_vnc_connected(si, (VncBasicInfo *)vs->info,
+ &error_abort);
break;
case QAPI_EVENT_VNC_INITIALIZED:
qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
@@ -356,11 +355,10 @@ static VncClientInfo *qmp_query_vnc_client(const VncState *client)
}
info = g_malloc0(sizeof(*info));
- info->base = g_malloc0(sizeof(*info->base));
- info->base->host = g_strdup(host);
- info->base->service = g_strdup(serv);
- info->base->family = inet_netfamily(sa.ss_family);
- info->base->websocket = client->websocket;
+ info->host = g_strdup(host);
+ info->service = g_strdup(serv);
+ info->family = inet_netfamily(sa.ss_family);
+ info->websocket = client->websocket;
#ifdef CONFIG_VNC_TLS
if (client->tls.session && client->tls.dname) {
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 04/20] qapi-visit: Remove redundant functions for flat union base
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (2 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 03/20] qapi: Unbox base members Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 05/20] qapi: Test use of 'number' within alternates Eric Blake
` (15 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The code for visiting the base class of a child struct created
visit_type_Base_fields(); the code for visiting the base class
of a flat union created visit_type_Union_fields(). If the same
type is shared between a struct and a union, the two functions
differed only by whether they visited the discriminator used by
the union. But if the base class always visits all its fields,
then we don't need to revisit the discriminator specially for
a union. By consistently visiting the base class fields under
the name of the base class, we can eliminate some redundant
code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-visit.py | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index ef96345..b131c0d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -115,7 +115,7 @@ if (err) {
''')
pop_indent()
- if re.search('^ *goto out;', ret, re.MULTILINE):
+ if base or members:
ret += mcgen('''
out:
@@ -241,8 +241,8 @@ def gen_visit_union(name, base, variants):
ret = ''
if base:
- members = [m for m in base.members if m != variants.tag_member]
- ret += gen_visit_struct_fields(name, None, members)
+ ret += gen_visit_struct_fields(base.name, base.base,
+ base.local_members)
for var in variants.variants:
# TODO ugly special case for simple union
@@ -263,29 +263,31 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
''',
c_name=c_name(name), name=name)
- if base:
- ret += mcgen('''
- visit_type_%(c_name)s_fields(m, obj, &err);
- if (err) {
- goto out_obj;
- }
-''',
- c_name=c_name(name))
-
tag_key = variants.tag_member.name
- ret += mcgen('''
+ if base:
+ ret += mcgen('''
+ visit_type_%(c_name)s_fields(m, (%(c_name)s **)obj, &err);
+ if (err) {
+ goto out_obj;
+ }
+''',
+ c_name=c_name(base.name))
+ else:
+ ret += mcgen('''
visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
if (err) {
goto out_obj;
}
+''',
+ c_type=variants.tag_member.type.c_name(),
+ c_name=c_name(tag_key), name=tag_key)
+ ret += mcgen('''
if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
goto out_obj;
}
switch ((*obj)->%(c_name)s) {
''',
- c_type=variants.tag_member.type.c_name(),
- c_name=c_name(variants.tag_member.name),
- name=tag_key)
+ c_name=c_name(tag_key))
for var in variants.variants:
# TODO ugly special case for simple union
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 05/20] qapi: Test use of 'number' within alternates
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (3 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 04/20] qapi-visit: Remove redundant functions for flat union base Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 06/20] qapi: Simplify visiting of alternate types Eric Blake
` (14 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Add some testsuite exposure for use of a 'number' as part of
an alternate. The current state of the tree has a few bugs
exposed by this: our input parser depends on the ordering of
how the qapi schema declared the alternate, and the parser
does not accept integers for a 'number' in an alternate even
though it does for numbers outside of an alternate.
Mixing 'int' and 'number' in the same alternate is unusual,
since both are supplied by json-numbers, but there does not
seem to be a technical reason to forbid it given that our
json lexer distinguishes between json-numbers that can be
represented as an int vs. those that cannot.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/qapi-schema/qapi-schema-test.json | 8 ++
tests/qapi-schema/qapi-schema-test.out | 24 ++++++
tests/test-qmp-input-visitor.c | 129 +++++++++++++++++++++++++++++++-
3 files changed, 158 insertions(+), 3 deletions(-)
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 6665281..cfbe4dd 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -59,6 +59,14 @@
{ 'struct': 'UserDefC',
'data': { 'string1': 'str', 'string2': 'str' } }
+# for testing use of 'number' within alternates
+{ 'alternate': 'AltOne', 'data': { 's': 'str', 'b': 'bool' } }
+{ 'alternate': 'AltTwo', 'data': { 's': 'str', 'n': 'number' } }
+{ 'alternate': 'AltThree', 'data': { 'n': 'number', 's': 'str' } }
+{ 'alternate': 'AltFour', 'data': { 's': 'str', 'i': 'int' } }
+{ 'alternate': 'AltFive', 'data': { 'i': 'int', 'n': 'number' } }
+{ 'alternate': 'AltSix', 'data': { 'n': 'number', 'i': 'int' } }
+
# for testing native lists
{ 'union': 'UserDefNativeListUnion',
'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 386b057..ba70ea7 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -51,6 +51,30 @@ object :obj-user_def_cmd2-arg
object :obj-user_def_cmd3-arg
member a: int optional=False
member b: int optional=True
+alternate AltFive
+ case i: int
+ case n: number
+enum AltFiveKind ['i', 'n']
+alternate AltFour
+ case s: str
+ case i: int
+enum AltFourKind ['s', 'i']
+alternate AltOne
+ case s: str
+ case b: bool
+enum AltOneKind ['s', 'b']
+alternate AltSix
+ case n: number
+ case i: int
+enum AltSixKind ['n', 'i']
+alternate AltThree
+ case n: number
+ case s: str
+enum AltThreeKind ['n', 's']
+alternate AltTwo
+ case s: str
+ case n: number
+enum AltTwoKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-arg
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index b6deee3..3ee1a1a 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -368,15 +368,136 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
{
Visitor *v;
Error *err = NULL;
- UserDefAlternate *tmp;
+ UserDefAlternate *tmp = NULL;
v = visitor_input_test_init(data, "42");
- visit_type_UserDefAlternate(v, &tmp, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
g_assert_cmpint(tmp->i, ==, 42);
qapi_free_UserDefAlternate(tmp);
+ tmp = NULL;
+
+ v = visitor_input_test_init(data, "'string'");
+
+ visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
+ g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
+ g_assert_cmpstr(tmp->s, ==, "string");
+ qapi_free_UserDefAlternate(tmp);
+ tmp = NULL;
+
+ v = visitor_input_test_init(data, "false");
+
+ visit_type_UserDefAlternate(v, &tmp, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_UserDefAlternate(tmp);
+}
+
+static void test_visitor_in_alternate_number(TestInputVisitorData *data,
+ const void *unused)
+{
+ Visitor *v;
+ Error *err = NULL;
+ AltOne *one = NULL;
+ AltTwo *two = NULL;
+ AltThree *three = NULL;
+ AltFour *four = NULL;
+ AltFive *five = NULL;
+ AltSix *six = NULL;
+
+ /* Parsing an int */
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltOne(v, &one, NULL, &err);
+ g_assert(err);
+ qapi_free_AltOne(one);
+ one = NULL;
+
+ /* FIXME: Integers should parse as numbers */
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltTwo(v, &two, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_AltTwo(two);
+ one = NULL;
+
+ /* FIXME: Order of alternate should not affect semantics */
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltThree(v, &three, NULL, &error_abort);
+ g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N);
+ g_assert_cmpfloat(three->n, ==, 42);
+ qapi_free_AltThree(three);
+ one = NULL;
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltFour(v, &four, NULL, &error_abort);
+ g_assert_cmpint(four->type, ==, ALT_FOUR_KIND_I);
+ g_assert_cmpint(four->i, ==, 42);
+ qapi_free_AltFour(four);
+ one = NULL;
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltFive(v, &five, NULL, &error_abort);
+ g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_I);
+ g_assert_cmpint(five->i, ==, 42);
+ qapi_free_AltFive(five);
+ one = NULL;
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltSix(v, &six, NULL, &error_abort);
+ g_assert_cmpint(six->type, ==, ALT_SIX_KIND_I);
+ g_assert_cmpint(six->i, ==, 42);
+ qapi_free_AltSix(six);
+ one = NULL;
+
+ /* Parsing a double */
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltOne(v, &one, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_AltOne(one);
+ one = NULL;
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltTwo(v, &two, NULL, &error_abort);
+ g_assert_cmpint(two->type, ==, ALT_TWO_KIND_N);
+ g_assert_cmpfloat(two->n, ==, 42.5);
+ qapi_free_AltTwo(two);
+ two = NULL;
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltThree(v, &three, NULL, &error_abort);
+ g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N);
+ g_assert_cmpfloat(three->n, ==, 42.5);
+ qapi_free_AltThree(three);
+ three = NULL;
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltFour(v, &four, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_AltFour(four);
+ four = NULL;
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltFive(v, &five, NULL, &error_abort);
+ g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_N);
+ g_assert_cmpfloat(five->n, ==, 42.5);
+ qapi_free_AltFive(five);
+ five = NULL;
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltSix(v, &six, NULL, &error_abort);
+ g_assert_cmpint(six->type, ==, ALT_SIX_KIND_N);
+ g_assert_cmpint(six->n, ==, 42.5);
+ qapi_free_AltSix(six);
+ six = NULL;
}
static void test_native_list_integer_helper(TestInputVisitorData *data,
@@ -720,6 +841,8 @@ int main(int argc, char **argv)
&in_visitor_data, test_visitor_in_alternate);
input_visitor_test_add("/visitor/input/errors",
&in_visitor_data, test_visitor_in_errors);
+ input_visitor_test_add("/visitor/input/alternate-number",
+ &in_visitor_data, test_visitor_in_alternate_number);
input_visitor_test_add("/visitor/input/native_list/int",
&in_visitor_data,
test_visitor_in_native_list_int);
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 06/20] qapi: Simplify visiting of alternate types
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (4 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 05/20] qapi: Test use of 'number' within alternates Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 07/20] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
` (13 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Previously, working with alternates required two enums, and
some indirection: for type Foo, we created Foo_qtypes[] which
maps each qtype to a member of FooKind_lookup[], then use
FooKind_lookup[] like we do for other union types.
This has a subtle bug: since the values of FooKind_lookup
start at zero, all entries of Foo_qtypes that were not
explicitly initialized map to the same branch of the union as
the first member of the alternate, rather than triggering a
failure in visit_get_next_type(). Fortunately, the bug
seldom bites; the very next thing the input visitor does is
try to parse the incoming JSON with the wrong parser, which
fails; the output visitor is not used with a C struct in that
state, and the dealloc visitor has nothing to clean up (so
there is no leak).
However, it IS observable in one case: the behavior of an
alternate that contains a 'number' member but no 'int' member
differs according to whether the 'number' was first in the
qapi definition, and when the input being parsed is an integer;
this is because the 'number' parser accepts QTYPE_QINT in
addition to the expected QTYPE_QFLOAT. A later patch will worry
about fixing alternates to parse all inputs that a non-alternate
'number' would accept.
This patch fixes the validation bug by deleting the indirection,
and modifying get_next_type() to directly return a qtype code.
There is no longer a need to generate an implicit FooKind array
associated with the alternate type (since the QMP wire format
never uses the stringized counterparts of the C union member
names). Next, the generated visitor is fixed to properly detect
unexpected qtypes in the switch statement. Things got a bit
tricky with validating QAPISchemaObjectTypeVariants, which now
has three different initialization paths; but I didn't think it
was confusing enough to need to create different sub-classes.
Callers now have to know the QTYPE_* mapping when looking at the
discriminator; but so far, only the testsuite was even using the
C struct of an alternate types. If that gets too confusing, we
could reintroduce FooKind, but initialize it differently than
most generated arrays, as in:
typedef enum FooKind {
FOO_KIND_A = QTYPE_QDICT,
FOO_KIND_B = QTYPE_QINT,
} FooKind;
to create nicer aliases for knowing when to use foo->a or foo->b
when inspecting foo->kind. But without a current client, I
didn't see the point of doing it now.
There is a user-visible side effect to this change, but I
consider it to be an improvement. Previously,
the invalid QMP command:
{"execute":"blockdev-add", "arguments":{"options":
{"driver":"raw", "id":"a", "file":true}}}
failed with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: QDict"}}
Now it fails with:
{"error": {"class": "GenericError",
"desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/qapi-code-gen.txt | 3 ---
include/qapi/visitor-impl.h | 2 +-
include/qapi/visitor.h | 2 +-
qapi/qapi-visit-core.c | 4 ++--
qapi/qmp-input-visitor.c | 4 ++--
scripts/qapi-types.py | 39 ++++++----------------------------
scripts/qapi-visit.py | 12 ++++++-----
scripts/qapi.py | 21 +++++++++++-------
tests/qapi-schema/alternate-good.out | 1 -
tests/qapi-schema/qapi-schema-test.out | 8 -------
tests/test-qmp-input-visitor.c | 26 +++++++++++------------
tests/test-qmp-output-visitor.c | 21 +++++++++++++-----
12 files changed, 61 insertions(+), 82 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 3a78cf4..5dd08e5 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -372,9 +372,6 @@ where each branch of the union names a QAPI type. For example:
'data': { 'definition': 'BlockdevOptions',
'reference': 'str' } }
-Just like for a simple union, an implicit C enum 'NameKind' is created
-to enumerate the branches for the alternate 'Name'.
-
Unlike a union, the discriminator string is never passed on the wire
for the Client JSON Protocol. Instead, the value's JSON type serves
as an implicit discriminator, which in turn means that an alternate
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8c0ba57..7098b93 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -32,7 +32,7 @@ struct Visitor
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
- void (*get_next_type)(Visitor *v, int *kind, const int *qobjects,
+ void (*get_next_type)(Visitor *v, qtype_code *type,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index cfc19a6..f1ac5c4 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -41,7 +41,7 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
void visit_end_list(Visitor *v, Error **errp);
void visit_optional(Visitor *v, bool *present, const char *name,
Error **errp);
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+void visit_get_next_type(Visitor *v, qtype_code *type,
const char *name, Error **errp);
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 59ed506..3f24daa 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char *name,
}
}
-void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
+void visit_get_next_type(Visitor *v, qtype_code *type,
const char *name, Error **errp)
{
if (v->get_next_type) {
- v->get_next_type(v, obj, qtypes, name, errp);
+ v->get_next_type(v, type, name, errp);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 5dd9ed5..803ffad 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
qmp_input_pop(qiv, errp);
}
-static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
+static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -218,7 +218,7 @@ static void qmp_input_get_next_type(Visitor *v, int *kind, const int *qobjects,
error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
return;
}
- *kind = qobjects[qobject_type(qobj)];
+ *type = qobject_type(qobj);
}
static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 27ad0c1..b0785a5 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -88,36 +88,6 @@ struct %(c_name)s {
return ret
-def gen_alternate_qtypes_decl(name):
- return mcgen('''
-
-extern const int %(c_name)s_qtypes[];
-''',
- c_name=c_name(name))
-
-def gen_alternate_qtypes(name, variants):
- ret = mcgen('''
-
-const int %(c_name)s_qtypes[QTYPE_MAX] = {
-''',
- c_name=c_name(name))
-
- for var in variants.variants:
- qtype = var.type.alternate_qtype()
- assert qtype
-
- ret += mcgen('''
- [%(qtype)s] = %(enum_const)s,
-''',
- qtype=qtype,
- enum_const=c_enum_const(variants.tag_member.type.name,
- var.name))
-
- ret += mcgen('''
-};
-''')
- return ret
-
def gen_union(name, base, variants):
ret = mcgen('''
@@ -126,6 +96,10 @@ struct %(c_name)s {
c_name=c_name(name))
if base:
ret += gen_struct_fields([], base)
+ elif not variants.tag_member:
+ ret += mcgen('''
+ qtype_code type;
+''')
else:
ret += mcgen('''
%(c_type)s type;
@@ -144,7 +118,8 @@ struct %(c_name)s {
union { /* union tag is @%(c_name)s */
void *data;
''',
- c_name=c_name(variants.tag_member.name))
+ c_name=c_name((variants.tag_member and
+ variants.tag_member.name) or 'type'))
for var in variants.variants:
# TODO ugly special case for simple union
@@ -244,9 +219,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self._gen_type_cleanup(name)
def visit_alternate_type(self, name, info, variants):
self.fwdecl += gen_fwd_object_or_array(name)
- self.fwdefn += gen_alternate_qtypes(name, variants)
self.decl += gen_union(name, None, variants)
- self.decl += gen_alternate_qtypes_decl(name)
self._gen_type_cleanup(name)
# If you link code generated from multiple schemata, you want only one
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b131c0d..06a27c5 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -203,7 +203,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(m, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err);
+ visit_get_next_type(m, &(*obj)->type, name, &err);
if (err) {
goto out_end;
}
@@ -217,14 +217,14 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
break;
''',
- case=c_enum_const(variants.tag_member.type.name,
- var.name),
+ case=var.type.alternate_qtype(),
c_type=var.type.c_name(),
c_name=c_name(var.name))
ret += mcgen('''
default:
- abort();
+ error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+ "%(name)s");
}
out_end:
error_propagate(errp, err);
@@ -233,7 +233,8 @@ out_end:
out:
error_propagate(errp, err);
}
-''')
+''',
+ name=name)
return ret
@@ -427,6 +428,7 @@ fdef.write(mcgen('''
fdecl.write(mcgen('''
#include "qapi/visitor.h"
+#include "qapi/qmp/qerror.h"
#include "%(prefix)sqapi-types.h"
''',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 99ea61d..6f41ae0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -929,29 +929,35 @@ class QAPISchemaObjectTypeVariants(object):
for v in variants:
assert isinstance(v, QAPISchemaObjectTypeVariant)
self.tag_name = tag_name
- if tag_name:
+ if tag_name: # flat union
assert not tag_enum
self.tag_member = None
- else:
+ elif tag_enum: # simple union
self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
False)
+ else: # alternate
+ self.tag_member = None
self.variants = variants
def check(self, schema, members, seen):
if self.tag_name:
self.tag_member = seen[self.tag_name]
- else:
+ elif self.tag_member:
self.tag_member.check(schema, members, seen)
- assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ typ = None
+ if self.tag_member:
+ assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ typ = self.tag_member.type
for v in self.variants:
vseen = dict(seen)
- v.check(schema, self.tag_member.type, vseen)
+ v.check(schema, typ, vseen)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
def check(self, schema, tag_type, seen):
QAPISchemaObjectTypeMember.check(self, schema, [], seen)
- assert self.name in tag_type.values
+ if tag_type:
+ assert self.name in tag_type.values
# TODO try to get rid of .simple_union_type()
def simple_union_type(self):
if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
@@ -1146,10 +1152,9 @@ class QAPISchema(object):
data = expr['data']
variants = [self._make_variant(key, data[key])
for key in data.keys()]
- tag_enum = self._make_tag_enum(name, variants)
self._def_entity(QAPISchemaAlternateType(name, info,
QAPISchemaObjectTypeVariants(None,
- tag_enum,
+ None,
variants)))
self._make_array_type(name) # TODO really needed?
diff --git a/tests/qapi-schema/alternate-good.out b/tests/qapi-schema/alternate-good.out
index 65af727..d211aba 100644
--- a/tests/qapi-schema/alternate-good.out
+++ b/tests/qapi-schema/alternate-good.out
@@ -3,7 +3,6 @@ alternate Alt
case value: int
case string: Enum
case struct: Data
-enum AltKind ['value', 'string', 'struct']
object Data
member number: int optional=True
member name: str optional=True
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index ba70ea7..f062a15 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -54,27 +54,21 @@ object :obj-user_def_cmd3-arg
alternate AltFive
case i: int
case n: number
-enum AltFiveKind ['i', 'n']
alternate AltFour
case s: str
case i: int
-enum AltFourKind ['s', 'i']
alternate AltOne
case s: str
case b: bool
-enum AltOneKind ['s', 'b']
alternate AltSix
case n: number
case i: int
-enum AltSixKind ['n', 'i']
alternate AltThree
case n: number
case s: str
-enum AltThreeKind ['n', 's']
alternate AltTwo
case s: str
case n: number
-enum AltTwoKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-arg
@@ -95,7 +89,6 @@ alternate UserDefAlternate
case uda: UserDefA
case s: str
case i: int
-enum UserDefAlternateKind ['uda', 's', 'i']
object UserDefB
member intb: int optional=False
object UserDefC
@@ -158,7 +151,6 @@ event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
alternate __org.qemu_x-Alt
case __org.qemu_x-branch: str
case b: __org.qemu_x-Base
-enum __org.qemu_x-AltKind ['__org.qemu_x-branch', 'b']
object __org.qemu_x-Base
member __org.qemu_x-member1: __org.qemu_x-Enum optional=False
enum __org.qemu_x-Enum ['__org.qemu_x-value']
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 3ee1a1a..cd833a4 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -373,7 +373,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
- g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QINT);
g_assert_cmpint(tmp->i, ==, 42);
qapi_free_UserDefAlternate(tmp);
tmp = NULL;
@@ -381,7 +381,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
v = visitor_input_test_init(data, "'string'");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
- g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
+ g_assert_cmpint(tmp->type, ==, QTYPE_QSTRING);
g_assert_cmpstr(tmp->s, ==, "string");
qapi_free_UserDefAlternate(tmp);
tmp = NULL;
@@ -424,31 +424,31 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
qapi_free_AltTwo(two);
one = NULL;
- /* FIXME: Order of alternate should not affect semantics */
v = visitor_input_test_init(data, "42");
- visit_type_AltThree(v, &three, NULL, &error_abort);
- g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N);
- g_assert_cmpfloat(three->n, ==, 42);
+ visit_type_AltThree(v, &three, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
qapi_free_AltThree(three);
one = NULL;
v = visitor_input_test_init(data, "42");
visit_type_AltFour(v, &four, NULL, &error_abort);
- g_assert_cmpint(four->type, ==, ALT_FOUR_KIND_I);
+ g_assert_cmpint(four->type, ==, QTYPE_QINT);
g_assert_cmpint(four->i, ==, 42);
qapi_free_AltFour(four);
one = NULL;
v = visitor_input_test_init(data, "42");
visit_type_AltFive(v, &five, NULL, &error_abort);
- g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_I);
+ g_assert_cmpint(five->type, ==, QTYPE_QINT);
g_assert_cmpint(five->i, ==, 42);
qapi_free_AltFive(five);
one = NULL;
v = visitor_input_test_init(data, "42");
visit_type_AltSix(v, &six, NULL, &error_abort);
- g_assert_cmpint(six->type, ==, ALT_SIX_KIND_I);
+ g_assert_cmpint(six->type, ==, QTYPE_QINT);
g_assert_cmpint(six->i, ==, 42);
qapi_free_AltSix(six);
one = NULL;
@@ -465,14 +465,14 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltTwo(v, &two, NULL, &error_abort);
- g_assert_cmpint(two->type, ==, ALT_TWO_KIND_N);
+ g_assert_cmpint(two->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(two->n, ==, 42.5);
qapi_free_AltTwo(two);
two = NULL;
v = visitor_input_test_init(data, "42.5");
visit_type_AltThree(v, &three, NULL, &error_abort);
- g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N);
+ g_assert_cmpint(three->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(three->n, ==, 42.5);
qapi_free_AltThree(three);
three = NULL;
@@ -487,14 +487,14 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltFive(v, &five, NULL, &error_abort);
- g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_N);
+ g_assert_cmpint(five->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(five->n, ==, 42.5);
qapi_free_AltFive(five);
five = NULL;
v = visitor_input_test_init(data, "42.5");
visit_type_AltSix(v, &six, NULL, &error_abort);
- g_assert_cmpint(six->type, ==, ALT_SIX_KIND_N);
+ g_assert_cmpint(six->type, ==, QTYPE_QFLOAT);
g_assert_cmpint(six->n, ==, 42.5);
qapi_free_AltSix(six);
six = NULL;
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index ab4bc5d..2b5c8d5 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -510,20 +510,31 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
const void *unused)
{
QObject *arg;
- Error *err = NULL;
+ UserDefAlternate *tmp;
- UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate));
- tmp->type = USER_DEF_ALTERNATE_KIND_I;
+ tmp = g_malloc0(sizeof(UserDefAlternate));
+ tmp->type = QTYPE_QINT;
tmp->i = 42;
- visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
arg = qmp_output_get_qobject(data->qov);
g_assert(qobject_type(arg) == QTYPE_QINT);
g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42);
qapi_free_UserDefAlternate(tmp);
+
+ tmp = g_malloc0(sizeof(UserDefAlternate));
+ tmp->type = QTYPE_QSTRING;
+ tmp->s = g_strdup("hello");
+
+ visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
+ arg = qmp_output_get_qobject(data->qov);
+
+ g_assert(qobject_type(arg) == QTYPE_QSTRING);
+ g_assert_cmpstr(qstring_get_str(qobject_to_qstring(arg)), ==, "hello");
+
+ qapi_free_UserDefAlternate(tmp);
}
static void test_visitor_out_empty(TestOutputVisitorData *data,
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 07/20] qapi: Fix alternates that accept 'number' but not 'int'
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (5 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 06/20] qapi: Simplify visiting of alternate types Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 08/20] qapi: Don't pass pre-existing error to later call Eric Blake
` (12 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The QMP input visitor allows integral values to be assigned by
promotion to a QTYPE_QFLOAT. However, when parsing an alternate,
we did not take this into account, such that an alternate that
accepts 'number' but not 'int' would reject integral values.
With this patch, we now have the following desirable table:
alternate has case selected for
'int' 'number' QTYPE_QINT QTYPE_QFLOAT
no no error error
no yes 'number' 'number'
yes no 'int' error
yes yes 'int' 'number'
While it is unlikely that we will ever use 'number' in an
alternate other than in the testsuite, it never hurts to be
more precise in what we allow.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/qapi/visitor-impl.h | 2 +-
include/qapi/visitor.h | 2 +-
qapi/qapi-visit-core.c | 4 ++--
qapi/qmp-input-visitor.c | 4 ++++
scripts/qapi-visit.py | 9 +++++++--
tests/test-qmp-input-visitor.c | 15 ++++++---------
6 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7098b93..c94e5a1 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -32,7 +32,7 @@ struct Visitor
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
- void (*get_next_type)(Visitor *v, qtype_code *type,
+ void (*get_next_type)(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index f1ac5c4..6a93c87 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -41,7 +41,7 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
void visit_end_list(Visitor *v, Error **errp);
void visit_optional(Visitor *v, bool *present, const char *name,
Error **errp);
-void visit_get_next_type(Visitor *v, qtype_code *type,
+void visit_get_next_type(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp);
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 3f24daa..884fe94 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char *name,
}
}
-void visit_get_next_type(Visitor *v, qtype_code *type,
+void visit_get_next_type(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp)
{
if (v->get_next_type) {
- v->get_next_type(v, type, name, errp);
+ v->get_next_type(v, type, promote_int, name, errp);
}
}
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 803ffad..5310db5 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -209,6 +209,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
}
static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
+ bool promote_int,
const char *name, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
@@ -219,6 +220,9 @@ static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
return;
}
*type = qobject_type(qobj);
+ if (promote_int && *type == QTYPE_QINT) {
+ *type = QTYPE_QFLOAT;
+ }
}
static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 06a27c5..a3a81c2 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -193,6 +193,11 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error
c_name=c_name(name), name=name)
def gen_visit_alternate(name, variants):
+ promote_int = 'true'
+ for var in variants.variants:
+ if var.type.alternate_qtype() == 'QTYPE_QINT':
+ promote_int = 'false'
+
ret = mcgen('''
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
@@ -203,13 +208,13 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(m, &(*obj)->type, name, &err);
+ visit_get_next_type(m, &(*obj)->type, %(promote_int)s, name, &err);
if (err) {
goto out_end;
}
switch ((*obj)->type) {
''',
- c_name=c_name(name))
+ c_name=c_name(name), promote_int=promote_int)
for var in variants.variants:
ret += mcgen('''
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index cd833a4..bba7ab2 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -415,20 +415,17 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
qapi_free_AltOne(one);
one = NULL;
- /* FIXME: Integers should parse as numbers */
v = visitor_input_test_init(data, "42");
- visit_type_AltTwo(v, &two, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_AltTwo(v, &two, NULL, &error_abort);
+ g_assert_cmpint(two->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(two->n, ==, 42.0);
qapi_free_AltTwo(two);
one = NULL;
v = visitor_input_test_init(data, "42");
- visit_type_AltThree(v, &three, NULL, &err);
- g_assert(err);
- error_free(err);
- err = NULL;
+ visit_type_AltThree(v, &three, NULL, &error_abort);
+ g_assert_cmpint(three->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(three->n, ==, 42.0);
qapi_free_AltThree(three);
one = NULL;
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 08/20] qapi: Don't pass pre-existing error to later call
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (6 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 07/20] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 09/20] qapi: Use consistent generated code patterns Eric Blake
` (11 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Due to the existing semantics of the error_set() family,
generated sequences in the qapi visitors such as:
visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
if (!err) {
visit_type_FOO_fields(m, obj, errp);
visit_end_implicit_struct(m, &err);
}
error_propagate(errp, err);
are risky: if visit_type_FOO_fields() sets errp, and then
visit_end_implicit_struct() also encounters an error, the
attempt to overwrite the first error will cause an abort().
Obviously, we weren't triggering this situation (none of the
existing callbacks for visit_end_implicit_struct() currently
try to set an error), but it is better to not even cause the
problem in the first place.
Meanwhile, in spite of the poor documentation of the qapi
visitors, we want to guarantee that if a visit_start_*()
succeeds, then the matching visit_end_*() will be called.
The options are to either propagate and clear a local err
multiple times:
visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
if (!err) {
visit_type_FOO_fields(m, obj, &err);
if (err) {
error_propagate(errp, err);
err = NULL;
}
visit_end_implicit_struct(m, &err);
}
error_propagate(errp, err);
or, as this patch does, just pass in NULL to ignore further
errors once an error has occurred.
visit_start_implicit_struct(m, (void **)obj, sizeof(FOO), &err);
if (!err) {
visit_type_FOO_fields(m, obj, &err);
visit_end_implicit_struct(m, err ? NULL : &err);
}
error_propagate(errp, err);
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-visit.py | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a3a81c2..cea0414 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -49,8 +49,8 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
if (!err) {
- visit_type_%(c_type)s_fields(m, obj, errp);
- visit_end_implicit_struct(m, &err);
+ visit_type_%(c_type)s_fields(m, obj, &err);
+ visit_end_implicit_struct(m, err ? NULL : &err);
}
error_propagate(errp, err);
}
@@ -142,9 +142,9 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
if (!err) {
if (*obj) {
- visit_type_%(c_name)s_fields(m, obj, errp);
+ visit_type_%(c_name)s_fields(m, obj, &err);
}
- visit_end_struct(m, &err);
+ visit_end_struct(m, err ? NULL : &err);
}
error_propagate(errp, err);
}
@@ -173,9 +173,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err);
}
- error_propagate(errp, err);
- err = NULL;
- visit_end_list(m, &err);
+ visit_end_list(m, err ? NULL : &err);
out:
error_propagate(errp, err);
}
@@ -232,9 +230,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
"%(name)s");
}
out_end:
- error_propagate(errp, err);
- err = NULL;
- visit_end_implicit_struct(m, &err);
+ visit_end_implicit_struct(m, err ? NULL : &err);
out:
error_propagate(errp, err);
}
@@ -326,10 +322,8 @@ out_obj:
error_propagate(errp, err);
err = NULL;
visit_end_union(m, !!(*obj)->data, &err);
- error_propagate(errp, err);
- err = NULL;
}
- visit_end_struct(m, &err);
+ visit_end_struct(m, err ? NULL : &err);
out:
error_propagate(errp, err);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 09/20] qapi: Use consistent generated code patterns
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (7 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 08/20] qapi: Don't pass pre-existing error to later call Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 10/20] qapi: Add tests for empty unions Eric Blake
` (10 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We had a pointless difference in label names, and on whether
the generated code used:
if (*obj) {
stuff;
...
}
vs.
if (!*obj) {
goto out_obj;
}
stuff;
...
Pick a single style, as it will make it easier for later patches
to uniformly apply any updates to generated code.
No change in semantics to the generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-visit.py | 53 ++++++++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index cea0414..58afb2d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -208,7 +208,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
}
visit_get_next_type(m, &(*obj)->type, %(promote_int)s, name, &err);
if (err) {
- goto out_end;
+ goto out_obj;
}
switch ((*obj)->type) {
''',
@@ -229,7 +229,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"%(name)s");
}
-out_end:
+out_obj:
visit_end_implicit_struct(m, err ? NULL : &err);
out:
error_propagate(errp, err);
@@ -261,33 +261,35 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- if (*obj) {
+ if (!*obj) {
+ goto out_obj;
+ }
''',
c_name=c_name(name), name=name)
tag_key = variants.tag_member.name
if base:
ret += mcgen('''
- visit_type_%(c_name)s_fields(m, (%(c_name)s **)obj, &err);
- if (err) {
- goto out_obj;
- }
+ visit_type_%(c_name)s_fields(m, (%(c_name)s **)obj, &err);
+ if (err) {
+ goto out_obj;
+ }
''',
c_name=c_name(base.name))
else:
ret += mcgen('''
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
- if (err) {
- goto out_obj;
- }
+ visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
+ if (err) {
+ goto out_obj;
+ }
''',
c_type=variants.tag_member.type.c_name(),
c_name=c_name(tag_key), name=tag_key)
ret += mcgen('''
- if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
- goto out_obj;
- }
- switch ((*obj)->%(c_name)s) {
+ if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
+ goto out_obj;
+ }
+ switch ((*obj)->%(c_name)s) {
''',
c_name=c_name(tag_key))
@@ -295,34 +297,33 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
# TODO ugly special case for simple union
simple_union_type = var.simple_union_type()
ret += mcgen('''
- case %(case)s:
+ case %(case)s:
''',
case=c_enum_const(variants.tag_member.type.name, var.name))
if simple_union_type:
ret += mcgen('''
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+ visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
''',
c_type=simple_union_type.c_name(),
c_name=c_name(var.name))
else:
ret += mcgen('''
- visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
+ visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
ret += mcgen('''
- break;
+ break;
''')
ret += mcgen('''
- default:
- abort();
- }
-out_obj:
- error_propagate(errp, err);
- err = NULL;
- visit_end_union(m, !!(*obj)->data, &err);
+ default:
+ abort();
}
+out_obj:
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_union(m, !!(*obj)->data, &err);
visit_end_struct(m, err ? NULL : &err);
out:
error_propagate(errp, err);
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 10/20] qapi: Add tests for empty unions
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (8 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 09/20] qapi: Use consistent generated code patterns Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 11/20] qapi: Rework deallocation of partial struct Eric Blake
` (9 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The documentation claims that alternates are useful for
allowing two types, although nothing enforces this. Meanwhile,
it is silent on whether empty unions are allowed. In practice,
the generated code will compile, in part because we have a
'void *data' branch; but for an empty alternate it always
fails (at least as of my recent fix to use qtypes directly), and
an empty union (whether simple or flat) always causes an abort().
Add some tests to expose the problems, and adjust existing tests
that should be failing for other reasons.
Not tested here: we have a namespace collision, where if the qapi
schema lists 'data' as one of the possible branch names of a
union, then the generated C code fails to compile due to a
duplicate member 'data'.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/Makefile | 1 +
tests/qapi-schema/alternate-empty.err | 0
tests/qapi-schema/alternate-empty.exit | 1 +
tests/qapi-schema/alternate-empty.json | 2 ++
tests/qapi-schema/alternate-empty.out | 3 +++
tests/qapi-schema/alternate-nested.json | 2 +-
tests/qapi-schema/alternate-unknown.json | 2 +-
tests/qapi-schema/flat-union-empty.err | 0
tests/qapi-schema/flat-union-empty.exit | 1 +
tests/qapi-schema/flat-union-empty.json | 4 ++++
tests/qapi-schema/flat-union-empty.out | 7 +++++++
tests/qapi-schema/union-empty.err | 0
tests/qapi-schema/union-empty.exit | 1 +
tests/qapi-schema/union-empty.json | 2 ++
tests/qapi-schema/union-empty.out | 3 +++
15 files changed, 27 insertions(+), 2 deletions(-)
create mode 100644 tests/qapi-schema/alternate-empty.err
create mode 100644 tests/qapi-schema/alternate-empty.exit
create mode 100644 tests/qapi-schema/alternate-empty.json
create mode 100644 tests/qapi-schema/alternate-empty.out
create mode 100644 tests/qapi-schema/flat-union-empty.err
create mode 100644 tests/qapi-schema/flat-union-empty.exit
create mode 100644 tests/qapi-schema/flat-union-empty.json
create mode 100644 tests/qapi-schema/flat-union-empty.out
create mode 100644 tests/qapi-schema/union-empty.err
create mode 100644 tests/qapi-schema/union-empty.exit
create mode 100644 tests/qapi-schema/union-empty.json
create mode 100644 tests/qapi-schema/union-empty.out
diff --git a/tests/Makefile b/tests/Makefile
index 398dc4a..8838f11 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -242,6 +242,7 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
unclosed-list.json unclosed-object.json unclosed-string.json \
duplicate-key.json union-invalid-base.json union-bad-branch.json \
union-optional-branch.json union-unknown.json union-max.json \
+ union-empty.json flat-union-empty.json alternate-empty.json \
flat-union-optional-discriminator.json flat-union-no-base.json \
flat-union-invalid-discriminator.json flat-union-inline.json \
flat-union-invalid-branch-key.json flat-union-reverse-define.json \
diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/alternate-empty.exit b/tests/qapi-schema/alternate-empty.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/alternate-empty.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json
new file mode 100644
index 0000000..db3820f
--- /dev/null
+++ b/tests/qapi-schema/alternate-empty.json
@@ -0,0 +1,2 @@
+# FIXME - alternates should list at least two types to be useful
+{ 'alternate': 'Alt', 'data': { 'i': 'int' } }
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
new file mode 100644
index 0000000..9b010d8
--- /dev/null
+++ b/tests/qapi-schema/alternate-empty.out
@@ -0,0 +1,3 @@
+object :empty
+alternate Alt
+ case i: int
diff --git a/tests/qapi-schema/alternate-nested.json b/tests/qapi-schema/alternate-nested.json
index c4233b9..8e22186 100644
--- a/tests/qapi-schema/alternate-nested.json
+++ b/tests/qapi-schema/alternate-nested.json
@@ -2,4 +2,4 @@
{ 'alternate': 'Alt1',
'data': { 'name': 'str', 'value': 'int' } }
{ 'alternate': 'Alt2',
- 'data': { 'nested': 'Alt1' } }
+ 'data': { 'nested': 'Alt1', 'b': 'bool' } }
diff --git a/tests/qapi-schema/alternate-unknown.json b/tests/qapi-schema/alternate-unknown.json
index ad5c103..08c80dc 100644
--- a/tests/qapi-schema/alternate-unknown.json
+++ b/tests/qapi-schema/alternate-unknown.json
@@ -1,3 +1,3 @@
# we reject an alternate with unknown type in branch
{ 'alternate': 'Alt',
- 'data': { 'unknown': 'MissingType' } }
+ 'data': { 'unknown': 'MissingType', 'i': 'int' } }
diff --git a/tests/qapi-schema/flat-union-empty.err b/tests/qapi-schema/flat-union-empty.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-empty.exit b/tests/qapi-schema/flat-union-empty.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/flat-union-empty.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/flat-union-empty.json b/tests/qapi-schema/flat-union-empty.json
new file mode 100644
index 0000000..67dd297
--- /dev/null
+++ b/tests/qapi-schema/flat-union-empty.json
@@ -0,0 +1,4 @@
+# FIXME - flat unions should not be empty
+{ 'enum': 'Empty', 'data': [ ] }
+{ 'struct': 'Base', 'data': { 'type': 'Empty' } }
+{ 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }
diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out
new file mode 100644
index 0000000..0e0665a
--- /dev/null
+++ b/tests/qapi-schema/flat-union-empty.out
@@ -0,0 +1,7 @@
+object :empty
+object Base
+ member type: Empty optional=False
+enum Empty []
+object Union
+ base Base
+ tag type
diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-empty.exit b/tests/qapi-schema/union-empty.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/union-empty.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/union-empty.json b/tests/qapi-schema/union-empty.json
new file mode 100644
index 0000000..1785007
--- /dev/null
+++ b/tests/qapi-schema/union-empty.json
@@ -0,0 +1,2 @@
+# FIXME - unions should not be empty
+{ 'union': 'Union', 'data': { } }
diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out
new file mode 100644
index 0000000..8b5a7bf
--- /dev/null
+++ b/tests/qapi-schema/union-empty.out
@@ -0,0 +1,3 @@
+object :empty
+object Union
+enum UnionKind []
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 11/20] qapi: Rework deallocation of partial struct
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (9 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 10/20] qapi: Add tests for empty unions Eric Blake
@ 2015-08-18 14:19 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 12/20] qapi: Avoid use of 'data' member of qapi unions Eric Blake
` (8 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 14:19 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Commit cee2dedb noticed that if you have a partial flat union
(such as if an input parse failed due to a missing
discriminator), calling the dealloc visitor could result in
trying to dereference the NULL pointer. But the fix it proposed
requires the use of a 'data' member in the union, which may or
may not be the same size as other branches of the union
(consider a 32-bit platform where one of the branches is an
int64), so it feels fairly dirty. A better, and much shorter,
fix is to tweak all of the generated visit_type_implicit_FOO()
functions to avoid dereferencing NULL in the first place, to
not visit the fields if the struct pointer itself is not present,
at which point we no longer even need visit_start_union(), and
no one was using visit_end_union() callbacks. Also, this fixes
things to guarantee that any successful call to
visit_start_implicit_struct() is paired with a matching
visit_end_implicit_struct().
The lack of documentation on the visitor interface is appalling,
but I'm not fixing it here.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/qapi/visitor-impl.h | 2 --
include/qapi/visitor.h | 2 --
qapi/qapi-dealloc-visitor.c | 26 --------------------------
qapi/qapi-visit-core.c | 15 ---------------
scripts/qapi-visit.py | 10 +++-------
5 files changed, 3 insertions(+), 52 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index c94e5a1..22539df 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -57,8 +57,6 @@ struct Visitor
void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
/* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
- bool (*start_union)(Visitor *v, bool data_present, Error **errp);
- void (*end_union)(Visitor *v, bool data_present, Error **errp);
};
void input_type_enum(Visitor *v, int *obj, const char * const strings[],
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 6a93c87..d1e853c 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -59,7 +59,5 @@ void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
void visit_type_any(Visitor *v, QObject **obj, const char *name, Error **errp);
-bool visit_start_union(Visitor *v, bool data_present, Error **errp);
-void visit_end_union(Visitor *v, bool data_present, Error **errp);
#endif
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 737deab..4989f50 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -171,31 +171,6 @@ static void qapi_dealloc_type_enum(Visitor *v, int *obj,
{
}
-/* If there's no data present, the dealloc visitor has nothing to free.
- * Thus, indicate to visitor code that the subsequent union fields can
- * be skipped. This is not an error condition, since the cleanup of the
- * rest of an object can continue unhindered, so leave errp unset in
- * these cases.
- *
- * NOTE: In cases where we're attempting to deallocate an object that
- * may have missing fields, the field indicating the union type may
- * be missing. In such a case, it's possible we don't have enough
- * information to differentiate data_present == false from a case where
- * data *is* present but happens to be a scalar with a value of 0.
- * This is okay, since in the case of the dealloc visitor there's no
- * work that needs to done in either situation.
- *
- * The current inability in QAPI code to more thoroughly verify a union
- * type in such cases will likely need to be addressed if we wish to
- * implement this interface for other types of visitors in the future,
- * however.
- */
-static bool qapi_dealloc_start_union(Visitor *v, bool data_present,
- Error **errp)
-{
- return data_present;
-}
-
Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
{
return &v->visitor;
@@ -226,7 +201,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
v->visitor.type_number = qapi_dealloc_type_number;
v->visitor.type_any = qapi_dealloc_type_anything;
v->visitor.type_size = qapi_dealloc_type_size;
- v->visitor.start_union = qapi_dealloc_start_union;
QTAILQ_INIT(&v->stack);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 884fe94..82bfdd0 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -58,21 +58,6 @@ void visit_end_list(Visitor *v, Error **errp)
v->end_list(v, errp);
}
-bool visit_start_union(Visitor *v, bool data_present, Error **errp)
-{
- if (v->start_union) {
- return v->start_union(v, data_present, errp);
- }
- return true;
-}
-
-void visit_end_union(Visitor *v, bool data_present, Error **errp)
-{
- if (v->end_union) {
- v->end_union(v, data_present, errp);
- }
-}
-
void visit_optional(Visitor *v, bool *present, const char *name,
Error **errp)
{
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 58afb2d..08dfd59 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -49,7 +49,9 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
if (!err) {
- visit_type_%(c_type)s_fields(m, obj, &err);
+ if (!obj || *obj) {
+ visit_type_%(c_type)s_fields(m, obj, &err);
+ }
visit_end_implicit_struct(m, err ? NULL : &err);
}
error_propagate(errp, err);
@@ -286,9 +288,6 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
c_type=variants.tag_member.type.c_name(),
c_name=c_name(tag_key), name=tag_key)
ret += mcgen('''
- if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
- goto out_obj;
- }
switch ((*obj)->%(c_name)s) {
''',
c_name=c_name(tag_key))
@@ -321,9 +320,6 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
abort();
}
out_obj:
- error_propagate(errp, err);
- err = NULL;
- visit_end_union(m, !!(*obj)->data, &err);
visit_end_struct(m, err ? NULL : &err);
out:
error_propagate(errp, err);
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 12/20] qapi: Avoid use of 'data' member of qapi unions
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (10 preceding siblings ...)
2015-08-18 14:19 ` [Qemu-devel] [PATCH RFC v3 11/20] qapi: Rework deallocation of partial struct Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 13/20] qapi: Forbid empty unions and useless alternates Eric Blake
` (7 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, armbru, open list:Block layer core, Gerd Hoffmann
qapi code generators currently create a 'void *data' member as
part of the anonymous union embedded in the C struct corresponding
to a qapi union. However, directly assigning to this member of
the union feels a bit fishy, when we can directly use the rest
of the struct instead.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
blockdev.c | 22 ++++++++++++----------
ui/input.c | 2 +-
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index fcaa63a..359012c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1052,14 +1052,11 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
}
}
-static void blockdev_do_action(int type, void *data, Error **errp)
+static void blockdev_do_action(TransactionAction *action, Error **errp)
{
- TransactionAction action;
TransactionActionList list;
- action.type = type;
- action.data = data;
- list.value = &action;
+ list.value = action;
list.next = NULL;
qmp_transaction(&list, errp);
}
@@ -1085,8 +1082,11 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
.has_mode = has_mode,
.mode = mode,
};
- blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
- &snapshot, errp);
+ TransactionAction action = {
+ .type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
+ .blockdev_snapshot_sync = &snapshot,
+ };
+ blockdev_do_action(&action, errp);
}
void qmp_blockdev_snapshot_internal_sync(const char *device,
@@ -1097,9 +1097,11 @@ void qmp_blockdev_snapshot_internal_sync(const char *device,
.device = (char *) device,
.name = (char *) name
};
-
- blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
- &snapshot, errp);
+ TransactionAction action = {
+ .type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+ .blockdev_snapshot_internal_sync = &snapshot,
+ };
+ blockdev_do_action(&action, errp);
}
SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
diff --git a/ui/input.c b/ui/input.c
index fd86571..edd237d 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -452,7 +452,7 @@ InputEvent *qemu_input_event_new_move(InputEventKind kind,
InputMoveEvent *move = g_new0(InputMoveEvent, 1);
evt->type = kind;
- evt->data = move;
+ evt->rel = move; /* also would work as evt->abs */
move->axis = axis;
move->value = value;
return evt;
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 13/20] qapi: Forbid empty unions and useless alternates
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (11 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 12/20] qapi: Avoid use of 'data' member of qapi unions Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 14/20] qapi: Drop useless 'data' member of unions Eric Blake
` (6 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Empty unions serve no purpose, and while we compile with gcc
which permits them, strict C99 forbids them. We could inject
a dummy member (and in fact, we do for empty structs), but while
empty structs make sense in qapi, empty unions don't add any
expressiveness to the QMP language. So prohibit them at parse
time. Update the documentation and testsuite to match.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/qapi-code-gen.txt | 13 +++++++------
scripts/qapi.py | 12 ++++++++++--
tests/qapi-schema/alternate-empty.err | 1 +
tests/qapi-schema/alternate-empty.exit | 2 +-
tests/qapi-schema/alternate-empty.json | 2 +-
tests/qapi-schema/alternate-empty.out | 3 ---
tests/qapi-schema/flat-union-empty.err | 1 +
tests/qapi-schema/flat-union-empty.exit | 2 +-
tests/qapi-schema/flat-union-empty.json | 2 +-
tests/qapi-schema/flat-union-empty.out | 7 -------
tests/qapi-schema/union-empty.err | 1 +
tests/qapi-schema/union-empty.exit | 2 +-
tests/qapi-schema/union-empty.json | 2 +-
tests/qapi-schema/union-empty.out | 3 ---
14 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 5dd08e5..688a60c 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -182,11 +182,11 @@ prevent incomplete include files.
Usage: { 'struct': STRING, 'data': DICT, '*base': STRUCT-NAME }
-A struct is a dictionary containing a single 'data' key whose
-value is a dictionary. This corresponds to a struct in C or an Object
-in JSON. Each value of the 'data' dictionary must be the name of a
-type, or a one-element array containing a type name. An example of a
-struct is:
+A struct is a dictionary containing a single 'data' key whose value is
+a dictionary; the dictionary may be empty. This corresponds to a
+struct in C or an Object in JSON. Each value of the 'data' dictionary
+must be the name of a type, or a one-element array containing a type
+name. An example of a struct is:
{ 'struct': 'MyType',
'data': { 'member1': 'str', 'member2': 'int', '*member3': 'str' } }
@@ -277,7 +277,8 @@ Union types are used to let the user choose between several different
variants for an object. There are two flavors: simple (no
discriminator or base), flat (both discriminator and base). A union
type is defined using a data dictionary as explained in the following
-paragraphs.
+paragraphs. The data dictionary for either type of union must not
+be empty.
A simple union type defines a mapping from automatic discriminator
values to data types like in this example:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6f41ae0..61e4c00 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -570,7 +570,10 @@ def check_union(expr, expr_info):
"Discriminator '%s' must be of enumeration "
"type" % discriminator)
- # Check every branch
+ # Check every branch; don't allow an empty union
+ if len(members) == 0:
+ raise QAPIExprError(expr_info,
+ "Union '%s' cannot have empty 'data'" % name)
for (key, value) in members.items():
check_name(expr_info, "Member of union '%s'" % name, key)
@@ -608,7 +611,11 @@ def check_alternate(expr, expr_info):
values = { 'MAX': '(automatic)' }
types_seen = {}
- # Check every branch
+ # Check every branch; require at least two branches
+ if len(members) < 2:
+ raise QAPIExprError(expr_info,
+ "Alternate '%s' should have at least two branches "
+ "in 'data'" % name)
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
@@ -926,6 +933,7 @@ class QAPISchemaObjectTypeVariants(object):
def __init__(self, tag_name, tag_enum, variants):
assert tag_name == None or isinstance(tag_name, str)
assert tag_enum == None or isinstance(tag_enum, str)
+ assert len(variants) > 0
for v in variants:
assert isinstance(v, QAPISchemaObjectTypeVariant)
self.tag_name = tag_name
diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err
index e69de29..bb06c5b 100644
--- a/tests/qapi-schema/alternate-empty.err
+++ b/tests/qapi-schema/alternate-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' should have at least two branches in 'data'
diff --git a/tests/qapi-schema/alternate-empty.exit b/tests/qapi-schema/alternate-empty.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/alternate-empty.exit
+++ b/tests/qapi-schema/alternate-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json
index db3820f..fff15ba 100644
--- a/tests/qapi-schema/alternate-empty.json
+++ b/tests/qapi-schema/alternate-empty.json
@@ -1,2 +1,2 @@
-# FIXME - alternates should list at least two types to be useful
+# alternates must list at least two types to be useful
{ 'alternate': 'Alt', 'data': { 'i': 'int' } }
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
index 9b010d8..e69de29 100644
--- a/tests/qapi-schema/alternate-empty.out
+++ b/tests/qapi-schema/alternate-empty.out
@@ -1,3 +0,0 @@
-object :empty
-alternate Alt
- case i: int
diff --git a/tests/qapi-schema/flat-union-empty.err b/tests/qapi-schema/flat-union-empty.err
index e69de29..15754f5 100644
--- a/tests/qapi-schema/flat-union-empty.err
+++ b/tests/qapi-schema/flat-union-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-empty.json:4: Union 'Union' cannot have empty 'data'
diff --git a/tests/qapi-schema/flat-union-empty.exit b/tests/qapi-schema/flat-union-empty.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-empty.exit
+++ b/tests/qapi-schema/flat-union-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-empty.json b/tests/qapi-schema/flat-union-empty.json
index 67dd297..77f1d9a 100644
--- a/tests/qapi-schema/flat-union-empty.json
+++ b/tests/qapi-schema/flat-union-empty.json
@@ -1,4 +1,4 @@
-# FIXME - flat unions should not be empty
+# flat unions cannot be empty
{ 'enum': 'Empty', 'data': [ ] }
{ 'struct': 'Base', 'data': { 'type': 'Empty' } }
{ 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }
diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out
index 0e0665a..e69de29 100644
--- a/tests/qapi-schema/flat-union-empty.out
+++ b/tests/qapi-schema/flat-union-empty.out
@@ -1,7 +0,0 @@
-object :empty
-object Base
- member type: Empty optional=False
-enum Empty []
-object Union
- base Base
- tag type
diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err
index e69de29..12c2022 100644
--- a/tests/qapi-schema/union-empty.err
+++ b/tests/qapi-schema/union-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-empty.json:2: Union 'Union' cannot have empty 'data'
diff --git a/tests/qapi-schema/union-empty.exit b/tests/qapi-schema/union-empty.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/union-empty.exit
+++ b/tests/qapi-schema/union-empty.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/union-empty.json b/tests/qapi-schema/union-empty.json
index 1785007..1f0b13c 100644
--- a/tests/qapi-schema/union-empty.json
+++ b/tests/qapi-schema/union-empty.json
@@ -1,2 +1,2 @@
-# FIXME - unions should not be empty
+# unions cannot be empty
{ 'union': 'Union', 'data': { } }
diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out
index 8b5a7bf..e69de29 100644
--- a/tests/qapi-schema/union-empty.out
+++ b/tests/qapi-schema/union-empty.out
@@ -1,3 +0,0 @@
-object :empty
-object Union
-enum UnionKind []
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 14/20] qapi: Drop useless 'data' member of unions
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (12 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 13/20] qapi: Forbid empty unions and useless alternates Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 15/20] qapi: Remove dead visitor code Eric Blake
` (5 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Now that we no longer have any clients of the 'void *data'
member injected into unions, we can drop it. Update the
testsuite to prove that we no longer have a name collision.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-types.py | 9 ---------
tests/qapi-schema/qapi-schema-test.json | 2 +-
tests/qapi-schema/qapi-schema-test.out | 4 ++--
3 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b0785a5..8f92b38 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -106,17 +106,8 @@ struct %(c_name)s {
''',
c_type=c_name(variants.tag_member.type.name))
- # FIXME: What purpose does data serve, besides preventing a union that
- # has a branch named 'data'? We use it in qapi-visit.py to decide
- # whether to bypass the switch statement if visiting the discriminator
- # failed; but since we 0-initialize structs, and cannot tell what
- # branch of the union is in use if the discriminator is invalid, there
- # should not be any data leaks even without a data pointer. Or, if
- # 'data' is merely added to guarantee we don't have an empty union,
- # shouldn't we enforce that at .json parse time?
ret += mcgen('''
union { /* union tag is @%(c_name)s */
- void *data;
''',
c_name=c_name((variants.tag_member and
variants.tag_member.name) or 'type'))
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index cfbe4dd..45143e3 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -81,7 +81,7 @@
'number': ['number'],
'boolean': ['bool'],
'string': ['str'],
- 'sizes': ['size'] } }
+ 'data': ['size'] } }
# testing commands
{ 'command': 'user_def_cmd', 'data': {} }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index f062a15..ce01fa7 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -119,8 +119,8 @@ object UserDefNativeListUnion
case number: :obj-numberList-wrapper
case boolean: :obj-boolList-wrapper
case string: :obj-strList-wrapper
- case sizes: :obj-sizeList-wrapper
-enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'sizes']
+ case data: :obj-sizeList-wrapper
+enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'data']
object UserDefOne
base UserDefZero
member string: str optional=False
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 15/20] qapi: Remove dead visitor code
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (13 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 14/20] qapi: Drop useless 'data' member of unions Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 16/20] qapi: Document visitor interfaces Eric Blake
` (4 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Commit cbc95538 removed unused start_handle() and end_handle(),
but forgot got remove their declarations.
Commit 4e27e819 introduced optional visitor callbacks for all
sorts of int types, but except for type_uint64 and type_size,
none of them have ever been supplied (the generic implementation
based on using type_int then bounds-checking works just fine).
In the interest of simplicity, it's easier to make the visitor
callback interface not have to worry about the other sizes.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/qapi/visitor-impl.h | 16 ++----
include/qapi/visitor.h | 3 -
qapi/qapi-visit-core.c | 131 +++++++++++++++++---------------------------
3 files changed, 56 insertions(+), 94 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 22539df..df70dd9 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -47,16 +47,12 @@ struct Visitor
void (*optional)(Visitor *v, bool *present, const char *name,
Error **errp);
- void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
- void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
- void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp);
- void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
- void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp);
- void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp);
- void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp);
- void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error **errp);
- /* visit_type_size() falls back to (*type_uint64)() if type_size is unset */
- void (*type_size)(Visitor *v, uint64_t *obj, const char *name, Error **errp);
+ /* visit_type_uint64() falls back to (*type_int)() as needed */
+ void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp);
+ /* visit_type_size() falls back to (*type_uint64)() as needed */
+ void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp);
};
void input_type_enum(Visitor *v, int *obj, const char * const strings[],
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index d1e853c..5436069 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,9 +27,6 @@ typedef struct GenericList
struct GenericList *next;
} GenericList;
-void visit_start_handle(Visitor *v, void **obj, const char *kind,
- const char *name, Error **errp);
-void visit_end_handle(Visitor *v, Error **errp);
void visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
void visit_end_struct(Visitor *v, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 82bfdd0..6d8ea95 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -89,57 +89,48 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_uint8) {
- v->type_uint8(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < 0 || value > UINT8_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint8_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < 0 || value > UINT8_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint8_t");
+ return;
}
+ *obj = value;
}
-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp)
+void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
+ Error **errp)
{
int64_t value;
- if (v->type_uint16) {
- v->type_uint16(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < 0 || value > UINT16_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint16_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < 0 || value > UINT16_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint16_t");
+ return;
}
+ *obj = value;
}
-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp)
+void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
+ Error **errp)
{
int64_t value;
- if (v->type_uint32) {
- v->type_uint32(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < 0 || value > UINT32_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "uint32_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < 0 || value > UINT32_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "uint32_t");
+ return;
}
+ *obj = value;
}
-void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
+void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp)
{
int64_t value;
@@ -156,77 +147,55 @@ void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_int8) {
- v->type_int8(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < INT8_MIN || value > INT8_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "int8_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < INT8_MIN || value > INT8_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "int8_t");
+ return;
}
+ *obj = value;
}
void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_int16) {
- v->type_int16(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < INT16_MIN || value > INT16_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "int16_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < INT16_MIN || value > INT16_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "int16_t");
+ return;
}
+ *obj = value;
}
void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp)
{
int64_t value;
- if (v->type_int32) {
- v->type_int32(v, obj, name, errp);
- } else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- if (value < INT32_MIN || value > INT32_MAX) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
- name ? name : "null", "int32_t");
- return;
- }
- *obj = value;
+ value = *obj;
+ v->type_int(v, &value, name, errp);
+ if (value < INT32_MIN || value > INT32_MAX) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "int32_t");
+ return;
}
+ *obj = value;
}
void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
{
- if (v->type_int64) {
- v->type_int64(v, obj, name, errp);
- } else {
- v->type_int(v, obj, name, errp);
- }
+ v->type_int(v, obj, name, errp);
}
void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
{
- int64_t value;
-
if (v->type_size) {
v->type_size(v, obj, name, errp);
- } else if (v->type_uint64) {
- v->type_uint64(v, obj, name, errp);
} else {
- value = *obj;
- v->type_int(v, &value, name, errp);
- *obj = value;
+ visit_type_uint64(v, obj, name, errp);
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 16/20] qapi: Document visitor interfaces
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (14 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 15/20] qapi: Remove dead visitor code Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 17/20] qapi: Change visit_type_XXX() to no longer return partial objects Eric Blake
` (3 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
The visitor interface for mapping between QObject and qapi
has formerly been documented only by reading source code,
making it difficult to propose changes to either
scripts/qapi*.py or to clients without knowing whether those
changes would be safe. This tries to add documentation,
including mentioning when parameters can be NULL, and where
there are still some interface warts that would be nice
to remove.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/qapi/visitor-impl.h | 39 ++++++++-
include/qapi/visitor.h | 187 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 213 insertions(+), 13 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index df70dd9..7e8f728 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -15,48 +15,79 @@
#include "qapi/error.h"
#include "qapi/visitor.h"
+/* This file describes the callback interface for implementing a
+ * QObject visitor. For the client interface, see visitor.h. When
+ * implementing the callbacks, it is easiest to declare a struct with
+ * 'Visitor visitor;' as the first member. Semantics for the
+ * callbacks are generally similar to the counterpart public
+ * interface. */
+
struct Visitor
{
- /* Must be set */
+ /* Must be provided to visit structs (the string visitors do not
+ * currently visit structs). */
void (*start_struct)(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
+ /* Must be provided if start_struct is present. */
void (*end_struct)(Visitor *v, Error **errp);
+ /* May be NULL; most useful for input visitors. */
void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
Error **errp);
+ /* May be NULL */
void (*end_implicit_struct)(Visitor *v, Error **errp);
+ /* Must be set */
void (*start_list)(Visitor *v, const char *name, Error **errp);
+ /* Must be set */
GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
+ /* Must be set */
void (*end_list)(Visitor *v, Error **errp);
+ /* Must be set, although the helpers input_type_enum() and
+ * output_type_enum() can be used. */
void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
+ /* May be NULL; most useful for input visitors. */
void (*get_next_type)(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp);
+ /* Must be set */
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);
+ /* Must be set */
void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **errp);
+ /* Must be set */
void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
+
+ /* Must be provided to visit numbers (the opts visitor does not
+ * currently visit non-integers). */
void (*type_number)(Visitor *v, double *obj, const char *name,
Error **errp);
+ /* Must be provided to visit arbitrary QTypes (the opts and string
+ * visitors do not currently visit arbitrary types). */
void (*type_any)(Visitor *v, QObject **obj, const char *name,
Error **errp);
- /* May be NULL */
+ /* May be NULL; most useful for input visitors. */
void (*optional)(Visitor *v, bool *present, const char *name,
Error **errp);
- /* visit_type_uint64() falls back to (*type_int)() as needed */
+ /* Only required to visit uint64 differently than (*type_int)(). */
void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
Error **errp);
- /* visit_type_size() falls back to (*type_uint64)() as needed */
+ /* Only required to visit sizes differently than (*type_uint64)(). */
void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
Error **errp);
};
+/**
+ * A generic visitor.type_enum suitable for input visitors.
+ */
void input_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
+/**
+ * A generic visitor.type_enum suitable for output visitors.
+ */
void output_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 5436069..d08115c 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,6 +18,17 @@
#include "qapi/error.h"
#include <stdlib.h>
+/* This file describes the client view for visiting a map between
+ * QObjects and another representation (command line options, strings,
+ * or generated QAPI C structs). An input visitor converts from
+ * QObject to another form; an output visitor converts from the other
+ * form back into QObjects. These functions seldom need to be called
+ * directly, but are instead used by code generated by
+ * scripts/qapi-visit.py. For the visitor callback contracts, see
+ * visitor-impl.h. */
+
+/* This struct is layout-compatible with all other *List structs
+ * created by the qapi generator. */
typedef struct GenericList
{
union {
@@ -27,34 +38,192 @@ typedef struct GenericList
struct GenericList *next;
} GenericList;
+/**
+ * Prepare to visit a QDict with C type @kind tied to QDict key @name.
+ * @name will be NULL if this is visited as part of a QList.
+ * The caller then makes a series of visit calls for each key expected
+ * in the dictionary, followed by a call to visit_end_struct(). For an
+ * input visitor, @obj can be NULL to validate that the visit will
+ * succeed; otherwise, *@obj is assigned with an allocation of @size
+ * bytes. For other visitors, *@obj is the object to visit. Set *@errp
+ * on failure.
+ *
+ * FIXME: *@obj can be modified even on error; this can lead to
+ * memory leaks if clients aren't careful.
+ */
void visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
+/**
+ * Complete the struct started by a previous visit_start_struct().
+ */
void visit_end_struct(Visitor *v, Error **errp);
+
+/**
+ * Prepare to visit an implicit struct.
+ * Similar to visit_start_struct(), except that this will visit a
+ * C pointer pointing to @size bytes, and where the QDict fields are
+ * part of the parent object.
+ *
+ * FIXME: *@obj can be modified even on error; this can lead to
+ * memory leaks if clients aren't careful.
+ */
void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
Error **errp);
+/**
+ * Complete the implicit struct started earlier.
+ * Only called when visit_start_implicit_struct() returns true.
+ */
void visit_end_implicit_struct(Visitor *v, Error **errp);
+
+/**
+ * Prepare to visit a QList tied to QDict key @name.
+ * @name will be NULL if this is visited as part of a QList.
+ * After calling this, the elements must be collected until
+ * visit_next_list() returns NULL, then visit_end_list() must be
+ * used to complete the visit.
+ */
void visit_start_list(Visitor *v, const char *name, Error **errp);
+/**
+ * Collect the next list member and append it to *@list.
+ * Must be called in a loop until a NULL return or error occurs;
+ * for each list member visited, the caller must also call the
+ * appropriate visit_type_*() for the element type of the list,
+ * with that function's name parameter set to NULL.
+ */
GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
+/**
+ * Complete the list started earlier.
+ * Must be called after visit_start_list(), even if there is a failure
+ * parsing list members.
+ */
void visit_end_list(Visitor *v, Error **errp);
+
+/**
+ * Check if an optional member @name of a QDict needs visiting.
+ * For input visitors, set *@present according to whether the
+ * corresponding visit_type_*() needs calling; for other visitors,
+ * leave *@present unchanged.
+ */
void visit_optional(Visitor *v, bool *present, const char *name,
Error **errp);
+
+/**
+ * Determine the qtype of the item @name in the current QDict visit.
+ * For input visitors, set *@type to the correct qtype of a qapi
+ * alternate type; for other visitors, leave *@type unchanged.
+ * If @promote_int, treat integers as numbers.
+ */
void visit_get_next_type(Visitor *v, qtype_code *type, bool promote_int,
const char *name, Error **errp);
+
+/**
+ * Visit an enum value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * For input visitors, parse a string and set *@obj to the numeric value
+ * of the enum type @kind using @strings as the mapping; for output
+ * visitors, reverse the mapping and visit the output string determined
+ * by *@obj.
+ */
void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
+
+/**
+ * Visit an integer value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * For input visitors, set *@obj to the parsed value; for other visitors,
+ * leave *@obj unchanged.
+ */
void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp);
-void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp);
-void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp);
-void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp);
-void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp);
+/**
+ * Visit a uint8_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to uint8_t range.
+ */
+void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name,
+ Error **errp);
+/**
+ * Visit a uint16_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to uint16_t range.
+ */
+void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
+ Error **errp);
+/**
+ * Visit a uint32_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to uint32_t range.
+ */
+void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
+ Error **errp);
+/**
+ * Visit a uint64_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to uint64_t range
+ * (that is, ensures it is unsigned).
+ */
+void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp);
+/**
+ * Visit an int8_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to int8_t range.
+ */
void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp);
-void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp);
-void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp);
-void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp);
-void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp);
+/**
+ * Visit an int16_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to int16_t range.
+ */
+void visit_type_int16(Visitor *v, int16_t *obj, const char *name,
+ Error **errp);
+/**
+ * Visit an uint32_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to int32_t range.
+ */
+void visit_type_int32(Visitor *v, int32_t *obj, const char *name,
+ Error **errp);
+/**
+ * Visit an int64_t value tied to @name in the current QDict visit.
+ * Like visit_type_int(), except clamps the value to int64_t range.
+ */
+void visit_type_int64(Visitor *v, int64_t *obj, const char *name,
+ Error **errp);
+/**
+ * Visit a uint64_t value tied to @name in the current QDict visit.
+ * Like visit_type_uint64(), except that some visitors may choose to
+ * recognize additional suffixes for easily scaling input values.
+ */
+void visit_type_size(Visitor *v, uint64_t *obj, const char *name,
+ Error **errp);
+
+/**
+ * Visit a boolean value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
+
+/**
+ * Visit a string value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * @obj must be non-NULL. Input visitors set *@obj to the parsed string;
+ * while output visitors leave *@obj unchanged, except that a NULL *@obj
+ * must be treated the same as "".
+ *
+ * FIXME: Unfortunately not const-correct for output visitors.
+ */
void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
-void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
+
+/**
+ * Visit a number value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
+void visit_type_number(Visitor *v, double *obj, const char *name,
+ Error **errp);
+
+/**
+ * Visit an arbitrary qtype value tied to @name in the current QDict visit.
+ * @name will be NULL if this is visited as part of a QList.
+ * Input visitors set *@obj to the value; other visitors will leave
+ * *@obj unchanged.
+ */
void visit_type_any(Visitor *v, QObject **obj, const char *name, Error **errp);
#endif
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 17/20] qapi: Change visit_type_XXX() to no longer return partial objects
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (15 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 16/20] qapi: Document visitor interfaces Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 18/20] qapi: Plumb in 'box' to qapi generator lower levels Eric Blake
` (2 subsequent siblings)
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Returning a partial object on error is an invitation for a careless
caller to leak memory. As no one outside the testsuite was actually
relying on these semantics, it is cleaner to just document and
guarantee that ALL visit_type_XXX() functions do not alter *obj
when an error is encountered during an input visitor.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/qapi/visitor.h | 40 ++++++++++++++++++++++++++++------------
qapi/qapi-visit-core.c | 8 ++++++--
scripts/qapi-visit.py | 37 +++++++++++++++++++++++++++----------
tests/test-qmp-commands.c | 15 +++++++--------
4 files changed, 68 insertions(+), 32 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index d08115c..cd1431a 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,6 +27,25 @@
* scripts/qapi-visit.py. For the visitor callback contracts, see
* visitor-impl.h. */
+/* All qapi types have a corresponding function with a signature
+ * compatible with this:
+ *
+ * void visit_type_XXX(Visitor *v, void *obj, const char *name, Error **errp);
+ *
+ * where *@obj is itself a pointer or a scalar. (The visit functions for
+ * built-in types are declared here, while the functions for qapi-defined
+ * struct, union, and enum types are generated; see qapi-visit.h). Input
+ * visitors populate *@obj on success, and leave it unchanged on failure.
+ *
+ * Additionally, all qapi structs have a generated function compatible
+ * with this:
+ *
+ * void qapi_free_XXX(void *obj);
+ *
+ * which behaves like free(), even if @obj is NULL or was only partially
+ * allocated before encountering an error.
+ */
+
/* This struct is layout-compatible with all other *List structs
* created by the qapi generator. */
typedef struct GenericList
@@ -46,12 +65,12 @@ typedef struct GenericList
* input visitor, @obj can be NULL to validate that the visit will
* succeed; otherwise, *@obj is assigned with an allocation of @size
* bytes. For other visitors, *@obj is the object to visit. Set *@errp
- * on failure.
- *
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * on failure. Returns true if *@obj was allocated; if that happens,
+ * and an error occurs any time before the matching visit_end_struct(),
+ * then the caller (usually a visit_type_XXX() function) knows to undo
+ * the allocation before returning control further.
*/
-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
/**
* Complete the struct started by a previous visit_start_struct().
@@ -60,14 +79,11 @@ void visit_end_struct(Visitor *v, Error **errp);
/**
* Prepare to visit an implicit struct.
- * Similar to visit_start_struct(), except that this will visit a
- * C pointer pointing to @size bytes, and where the QDict fields are
- * part of the parent object.
- *
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * Similar to visit_start_struct(), including return semantics, except
+ * that this will visit a C pointer pointing to @size bytes, and where
+ * the QDict fields are part of the parent object.
*/
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
Error **errp);
/**
* Complete the implicit struct started earlier.
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6d8ea95..9cd17f8 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -17,10 +17,12 @@
#include "qapi/visitor.h"
#include "qapi/visitor-impl.h"
-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp)
{
+ bool track_allocation = obj && !*obj;
v->start_struct(v, obj, kind, name, size, errp);
+ return track_allocation && *obj;
}
void visit_end_struct(Visitor *v, Error **errp)
@@ -28,12 +30,14 @@ void visit_end_struct(Visitor *v, Error **errp)
v->end_struct(v, errp);
}
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
Error **errp)
{
+ bool track_allocation = obj && !*obj;
if (v->start_implicit_struct) {
v->start_implicit_struct(v, obj, size, errp);
}
+ return track_allocation && *obj;
}
void visit_end_implicit_struct(Visitor *v, Error **errp)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 08dfd59..e4f7c89 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -46,14 +46,19 @@ static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **e
static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
+ allocated = visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
if (!err) {
if (!obj || *obj) {
visit_type_%(c_type)s_fields(m, obj, &err);
}
visit_end_implicit_struct(m, err ? NULL : &err);
}
+ if (allocated && err) {
+ g_free(*obj);
+ *obj = NULL;
+ }
error_propagate(errp, err);
}
''',
@@ -131,23 +136,24 @@ out:
def gen_visit_struct(name, base, members):
ret = gen_visit_struct_fields(name, base, members)
- # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
- # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
- # rather than leaving it non-NULL. As currently written, the caller must
- # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
ret += mcgen('''
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+ allocated = visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
if (!err) {
if (*obj) {
visit_type_%(c_name)s_fields(m, obj, &err);
}
visit_end_struct(m, err ? NULL : &err);
}
+ if (allocated && err) {
+ qapi_free_%(c_name)s(*obj);
+ *obj = NULL;
+ }
error_propagate(errp, err);
}
''',
@@ -203,8 +209,9 @@ def gen_visit_alternate(name, variants):
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
+ allocated = visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
if (err) {
goto out;
}
@@ -233,11 +240,15 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
}
out_obj:
visit_end_implicit_struct(m, err ? NULL : &err);
+ if (allocated && err) {
+ qapi_free_%(c_name)s(*obj);
+ *obj = NULL;
+ }
out:
error_propagate(errp, err);
}
''',
- name=name)
+ name=name, c_name=c_name(name))
return ret
@@ -258,8 +269,9 @@ def gen_visit_union(name, base, variants):
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+ allocated = visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
if (err) {
goto out;
}
@@ -321,10 +333,15 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
}
out_obj:
visit_end_struct(m, err ? NULL : &err);
+ if (allocated && err) {
+ qapi_free_%(c_name)s(*obj);
+ *obj = NULL;
+ }
out:
error_propagate(errp, err);
}
-''')
+''',
+ c_name=c_name(name))
return ret
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 5181823..89a3b47 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -218,15 +218,14 @@ static void test_dealloc_partial(void)
QDECREF(ud2_dict);
}
- /* verify partial success */
- assert(ud2 != NULL);
- assert(ud2->string0 != NULL);
- assert(strcmp(ud2->string0, text) == 0);
- assert(ud2->dict1 == NULL);
-
- /* confirm & release construction error */
- assert(err != NULL);
+ /* verify that visit_type_XXX() cleans up properly on error */
+ assert(err);
error_free(err);
+ assert(!ud2);
+
+ /* Manually create a partial object, leaving ud2->dict1 at NULL */
+ ud2 = g_new0(UserDefTwo, 1);
+ ud2->string0 = g_strdup(text);
/* tear down partial object */
qapi_free_UserDefTwo(ud2);
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 18/20] qapi: Plumb in 'box' to qapi generator lower levels
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (16 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 17/20] qapi: Change visit_type_XXX() to no longer return partial objects Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 19/20] qapi: Implement boxed structs for commands/events Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 20/20] qapi: Support boxed unions Eric Blake
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
A future patch will add support for passing a qapi union
type as the 'data' of a command. But to do that, the user
function for implementing the command, as called by the
generated marshal command, must take the corresponding C
struct as a single boxed pointer, rather than a breakdown
into one parameter per member. This patch adds the
internal plubming of a 'box' flag associated with each
command and event. For this patch, no behavior changes,
other than the testsuite outputting the value of the new
flag (always False for now).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-commands.py | 93 ++++++++++++++++++---------------
scripts/qapi-event.py | 65 ++++++++++++-----------
scripts/qapi-introspect.py | 4 +-
scripts/qapi.py | 47 ++++++++++-------
tests/qapi-schema/args-member-array.out | 2 +-
tests/qapi-schema/event-case.out | 1 +
tests/qapi-schema/ident-with-escape.out | 2 +-
tests/qapi-schema/indented-expr.out | 4 +-
tests/qapi-schema/qapi-schema-test.out | 17 +++---
tests/qapi-schema/returns-int.out | 2 +-
tests/qapi-schema/test-qapi.py | 8 +--
11 files changed, 139 insertions(+), 106 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 7ff7c31..e2b8f7a 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -15,13 +15,13 @@
from qapi import *
import re
-def gen_command_decl(name, arg_type, ret_type):
+def gen_command_decl(name, arg_type, box, ret_type):
return mcgen('''
%(c_type)s qmp_%(c_name)s(%(params)s);
''',
c_type=(ret_type and ret_type.c_type()) or 'void',
c_name=c_name(name),
- params=gen_params(arg_type, 'Error **errp'))
+ params=gen_params(arg_type, box, 'Error **errp'))
def gen_err_check(err):
if not err:
@@ -33,15 +33,18 @@ if (%(err)s) {
''',
err=err)
-def gen_call(name, arg_type, ret_type):
+def gen_call(name, arg_type, box, ret_type):
ret = ''
argstr = ''
- if arg_type:
- for memb in arg_type.members:
- if memb.optional:
- argstr += 'has_%s, ' % c_name(memb.name)
- argstr += '%s, ' % c_name(memb.name)
+ if box:
+ assert False # not implemented
+ else:
+ if arg_type:
+ for memb in arg_type.members:
+ if memb.optional:
+ argstr += 'has_%s, ' % c_name(memb.name)
+ argstr += '%s, ' % c_name(memb.name)
lhs = ''
if ret_type:
@@ -63,7 +66,7 @@ qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
pop_indent()
return ret
-def gen_marshal_vars(arg_type, ret_type):
+def gen_marshal_vars(arg_type, box, ret_type):
ret = mcgen('''
Error *local_err = NULL;
''')
@@ -83,18 +86,21 @@ QapiDeallocVisitor *md;
Visitor *v;
''')
- for memb in arg_type.members:
- if memb.optional:
- ret += mcgen('''
+ if box:
+ assert False # not implemented
+ else:
+ for memb in arg_type.members:
+ if memb.optional:
+ ret += mcgen('''
bool has_%(c_name)s = false;
''',
- c_name=c_name(memb.name))
- ret += mcgen('''
+ c_name=c_name(memb.name))
+ ret += mcgen('''
%(c_type)s %(c_name)s = %(c_null)s;
''',
- c_name=c_name(memb.name),
- c_type=memb.type.c_type(),
- c_null=memb.type.c_null())
+ c_name=c_name(memb.name),
+ c_type=memb.type.c_type(),
+ c_null=memb.type.c_null())
ret += '\n'
else:
ret += mcgen('''
@@ -105,7 +111,7 @@ bool has_%(c_name)s = false;
pop_indent()
return ret
-def gen_marshal_input_visit(arg_type, dealloc=False):
+def gen_marshal_input_visit(arg_type, box, dealloc=False):
ret = ''
if not arg_type:
@@ -128,28 +134,31 @@ v = qapi_dealloc_get_visitor(md);
v = qmp_input_get_visitor(mi);
''')
- for memb in arg_type.members:
- if memb.optional:
- ret += mcgen('''
+ if box:
+ assert False # not implemented
+ else:
+ for memb in arg_type.members:
+ if memb.optional:
+ ret += mcgen('''
visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
''',
- c_name=c_name(memb.name), name=memb.name,
- errp=errparg)
- ret += gen_err_check(errarg)
- ret += mcgen('''
+ c_name=c_name(memb.name), name=memb.name,
+ errp=errparg)
+ ret += gen_err_check(errarg)
+ ret += mcgen('''
if (has_%(c_name)s) {
''',
- c_name=c_name(memb.name))
- push_indent()
- ret += mcgen('''
+ c_name=c_name(memb.name))
+ push_indent()
+ ret += mcgen('''
visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
''',
- c_name=c_name(memb.name), name=memb.name,
- c_type=memb.type.c_name(), errp=errparg)
- ret += gen_err_check(errarg)
- if memb.optional:
- pop_indent()
- ret += mcgen('''
+ c_name=c_name(memb.name), name=memb.name,
+ c_type=memb.type.c_name(), errp=errparg)
+ ret += gen_err_check(errarg)
+ if memb.optional:
+ pop_indent()
+ ret += mcgen('''
}
''')
@@ -200,7 +209,7 @@ def gen_marshal_decl(name):
''',
proto=gen_marshal_proto(name))
-def gen_marshal(name, arg_type, ret_type):
+def gen_marshal(name, arg_type, box, ret_type):
ret = mcgen('''
%(proto)s
@@ -208,9 +217,9 @@ def gen_marshal(name, arg_type, ret_type):
''',
proto=gen_marshal_proto(name))
- ret += gen_marshal_vars(arg_type, ret_type)
- ret += gen_marshal_input_visit(arg_type)
- ret += gen_call(name, arg_type, ret_type)
+ ret += gen_marshal_vars(arg_type, box, ret_type)
+ ret += gen_marshal_input_visit(arg_type, box)
+ ret += gen_call(name, arg_type, box, ret_type)
if re.search('^ *goto out;', ret, re.MULTILINE):
ret += mcgen('''
@@ -220,7 +229,7 @@ out:
ret += mcgen('''
error_propagate(errp, local_err);
''')
- ret += gen_marshal_input_visit(arg_type, dealloc=True)
+ ret += gen_marshal_input_visit(arg_type, box, dealloc=True)
ret += mcgen('''
}
''')
@@ -271,16 +280,16 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
self.regy = None
self.visited_ret_types = None
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response):
+ gen, success_response, box):
if not gen:
return
- self.decl += gen_command_decl(name, arg_type, ret_type)
+ self.decl += gen_command_decl(name, arg_type, box, ret_type)
if ret_type and ret_type not in self.visited_ret_types:
self.visited_ret_types.add(ret_type)
self.defn += gen_marshal_output(ret_type)
if middle_mode:
self.decl += gen_marshal_decl(name)
- self.defn += gen_marshal(name, arg_type, ret_type)
+ self.defn += gen_marshal(name, arg_type, box, ret_type)
if not middle_mode:
self.regy += gen_register_command(name, success_response)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index d100099..278cd7f 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -13,19 +13,19 @@
from qapi import *
-def gen_event_send_proto(name, arg_type):
+def gen_event_send_proto(name, arg_type, box):
return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
'c_name': c_name(name.lower()),
- 'param': gen_params(arg_type, 'Error **errp')}
+ 'param': gen_params(arg_type, box, 'Error **errp')}
-def gen_event_send_decl(name, arg_type):
+def gen_event_send_decl(name, arg_type, box):
return mcgen('''
%(proto)s;
''',
- proto=gen_event_send_proto(name, arg_type))
+ proto=gen_event_send_proto(name, arg_type, box))
-def gen_event_send(name, arg_type):
+def gen_event_send(name, arg_type, box):
ret = mcgen('''
%(proto)s
@@ -34,7 +34,7 @@ def gen_event_send(name, arg_type):
Error *local_err = NULL;
QMPEventFuncEmit emit;
''',
- proto=gen_event_send_proto(name, arg_type))
+ proto=gen_event_send_proto(name, arg_type, box))
if arg_type and arg_type.members:
ret += mcgen('''
@@ -63,47 +63,52 @@ def gen_event_send(name, arg_type):
v = qmp_output_get_visitor(qov);
g_assert(v);
+''')
+
+ if box:
+ assert False # not implemented
+ else:
+ ret += mcgen('''
/* Fake visit, as if all members are under a structure */
visit_start_struct(v, NULL, "", "%(name)s", 0, &local_err);
if (local_err) {
goto clean;
}
-
''',
- name=name)
+ name=name)
- for memb in arg_type.members:
- if memb.optional:
- ret += mcgen('''
+ for memb in arg_type.members:
+ if memb.optional:
+ ret += mcgen('''
if (has_%(c_name)s) {
''',
- c_name=c_name(memb.name))
- push_indent()
+ c_name=c_name(memb.name))
+ push_indent()
- # Ugly: need to cast away the const
- if memb.type.name == "str":
- cast = '(char **)'
- else:
- cast = ''
+ # Ugly: need to cast away the const
+ if memb.type.name == "str":
+ cast = '(char **)'
+ else:
+ cast = ''
- ret += mcgen('''
+ ret += mcgen('''
visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &local_err);
if (local_err) {
goto clean;
}
''',
- cast=cast,
- c_name=c_name(memb.name),
- c_type=memb.type.c_name(),
- name=memb.name)
+ cast=cast,
+ c_name=c_name(memb.name),
+ c_type=memb.type.c_name(),
+ name=memb.name)
- if memb.optional:
- pop_indent()
- ret += mcgen('''
+ if memb.optional:
+ pop_indent()
+ ret += mcgen('''
}
''')
- ret += mcgen('''
+ ret += mcgen('''
visit_end_struct(v, &local_err);
if (local_err) {
@@ -147,9 +152,9 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
self.decl += gen_enum(event_enum_name, self.event_names)
self.defn += gen_enum_lookup(event_enum_name, self.event_names)
self.event_names = None
- def visit_event(self, name, info, arg_type):
- self.decl += gen_event_send_decl(name, arg_type)
- self.defn += gen_event_send(name, arg_type)
+ def visit_event(self, name, info, arg_type, box):
+ self.decl += gen_event_send_decl(name, arg_type, box)
+ self.defn += gen_event_send(name, arg_type, box)
self.event_names.append(name)
(input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line()
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index d723ef1..935ffc3 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -145,14 +145,14 @@ const char %(c_name)s[] = %(c_string)s;
for m in variants.variants] })
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response):
+ gen, success_response, box):
arg_type = arg_type or self.schema.the_empty_object_type
ret_type = ret_type or self.schema.the_empty_object_type
self._gen_json(name, 'command',
{ 'arg-type': self._use_type(arg_type),
'ret-type': self._use_type(ret_type) })
- def visit_event(self, name, info, arg_type):
+ def visit_event(self, name, info, arg_type, box):
arg_type = arg_type or self.schema.the_empty_object_type
self._gen_json(name, 'event', { 'arg-type': self._use_type(arg_type) })
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 61e4c00..313eaef 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -792,9 +792,9 @@ class QAPISchemaVisitor(object):
def visit_alternate_type(self, name, info, variants):
pass
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response):
+ gen, success_response, box):
pass
- def visit_event(self, name, info, arg_type):
+ def visit_event(self, name, info, arg_type, box):
pass
class QAPISchemaType(QAPISchemaEntity):
@@ -986,7 +986,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
visitor.visit_alternate_type(self.name, self.info, self.variants)
class QAPISchemaCommand(QAPISchemaEntity):
- def __init__(self, name, info, arg_type, ret_type, gen, success_response):
+ def __init__(self, name, info, arg_type, ret_type, gen, success_response,
+ box):
QAPISchemaEntity.__init__(self, name, info)
assert not arg_type or isinstance(arg_type, str)
assert not ret_type or isinstance(ret_type, str)
@@ -996,32 +997,36 @@ class QAPISchemaCommand(QAPISchemaEntity):
self.ret_type = None
self.gen = gen
self.success_response = success_response
+ self.box = box
def check(self, schema):
if self.arg_type_name:
self.arg_type = schema.lookup_type(self.arg_type_name)
assert isinstance(self.arg_type, QAPISchemaObjectType)
- assert not self.arg_type.variants # not implemented
+ if not self.box:
+ assert not self.arg_type.variants
if self.ret_type_name:
self.ret_type = schema.lookup_type(self.ret_type_name)
assert isinstance(self.ret_type, QAPISchemaType)
def visit(self, visitor):
visitor.visit_command(self.name, self.info,
self.arg_type, self.ret_type,
- self.gen, self.success_response)
+ self.gen, self.success_response, self.box)
class QAPISchemaEvent(QAPISchemaEntity):
- def __init__(self, name, info, arg_type):
+ def __init__(self, name, info, arg_type, box):
QAPISchemaEntity.__init__(self, name, info)
assert not arg_type or isinstance(arg_type, str)
self.arg_type_name = arg_type
self.arg_type = None
+ self.box = box
def check(self, schema):
if self.arg_type_name:
self.arg_type = schema.lookup_type(self.arg_type_name)
assert isinstance(self.arg_type, QAPISchemaObjectType)
- assert not self.arg_type.variants # not implemented
+ if not self.box:
+ assert not self.arg_type.variants
def visit(self, visitor):
- visitor.visit_event(self.name, self.info, self.arg_type)
+ visitor.visit_event(self.name, self.info, self.arg_type, self.box)
class QAPISchema(object):
def __init__(self, fname):
@@ -1172,6 +1177,7 @@ class QAPISchema(object):
rets = expr.get('returns')
gen = expr.get('gen', True)
success_response = expr.get('success-response', True)
+ box = expr.get('box', False)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(name, 'arg',
self._make_members(data))
@@ -1182,15 +1188,16 @@ class QAPISchema(object):
rets = self._make_implicit_object_type(name, 'ret',
self._make_members(rets))
self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
- success_response))
+ success_response, box))
def _def_event(self, expr, info):
name = expr['event']
data = expr.get('data')
+ box = expr.get('box', False)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(name, 'arg',
self._make_members(data))
- self._def_entity(QAPISchemaEvent(name, info, data))
+ self._def_entity(QAPISchemaEvent(name, info, data, box))
def _def_exprs(self):
for expr_elem in self.exprs:
@@ -1413,18 +1420,22 @@ extern const char *const %(c_name)s_lookup[];
c_name=c_name(name))
return ret
-def gen_params(arg_type, extra):
+def gen_params(arg_type, box, extra):
if not arg_type:
return extra
- assert not arg_type.variants
ret = ''
sep = ''
- for memb in arg_type.members:
- ret += sep
- sep = ', '
- if memb.optional:
- ret += 'bool has_%s, ' % c_name(memb.name)
- ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
+ if box:
+ assert False # not implemented
+ else:
+ assert not arg_type.variants
+ for memb in arg_type.members:
+ ret += sep
+ sep = ', '
+ if memb.optional:
+ ret += 'bool has_%s, ' % c_name(memb.name)
+ ret += '%s %s' % (memb.type.c_type(is_param=True),
+ c_name(memb.name))
if extra:
ret += sep + extra
return ret
diff --git a/tests/qapi-schema/args-member-array.out b/tests/qapi-schema/args-member-array.out
index b3b92df..f5dc409 100644
--- a/tests/qapi-schema/args-member-array.out
+++ b/tests/qapi-schema/args-member-array.out
@@ -6,4 +6,4 @@ enum abc ['a', 'b', 'c']
object def
member array: abcList optional=False
command okay :obj-okay-arg -> None
- gen=True success_response=True
+ gen=True success_response=True box=False
diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out
index cdfd264..97a682a 100644
--- a/tests/qapi-schema/event-case.out
+++ b/tests/qapi-schema/event-case.out
@@ -1,2 +1,3 @@
object :empty
event oops None
+ box=False
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index f4542b1..2381c33 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -2,4 +2,4 @@ object :empty
object :obj-fooA-arg
member bar1: str optional=False
command fooA :obj-fooA-arg -> None
- gen=True success_response=True
+ gen=True success_response=True box=False
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 226d300..f5d2695 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,5 +1,5 @@
object :empty
command eins None -> None
- gen=True success_response=True
+ gen=True success_response=True box=False
command zwei None -> None
- gen=True success_response=True
+ gen=True success_response=True box=False
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index ce01fa7..1a66627 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -70,9 +70,13 @@ alternate AltTwo
case s: str
case n: number
event EVENT_A None
+ box=False
event EVENT_B None
+ box=False
event EVENT_C :obj-EVENT_C-arg
+ box=False
event EVENT_D :obj-EVENT_D-arg
+ box=False
enum EnumOne ['value1', 'value2', 'value3']
object EventStructOne
member struct1: UserDefOne optional=False
@@ -148,6 +152,7 @@ object UserDefUnionBase
object UserDefZero
member integer: int optional=False
event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
+ box=False
alternate __org.qemu_x-Alt
case __org.qemu_x-branch: str
case b: __org.qemu_x-Base
@@ -167,14 +172,14 @@ object __org.qemu_x-Union2
tag __org.qemu_x-member1
case __org.qemu_x-value: __org.qemu_x-Struct2
command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> __org.qemu_x-Union1
- gen=True success_response=True
+ gen=True success_response=True box=False
command guest-sync :obj-guest-sync-arg -> any
- gen=True success_response=True
+ gen=True success_response=True box=False
command user_def_cmd None -> None
- gen=True success_response=True
+ gen=True success_response=True box=False
command user_def_cmd1 :obj-user_def_cmd1-arg -> None
- gen=True success_response=True
+ gen=True success_response=True box=False
command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
- gen=True success_response=True
+ gen=True success_response=True box=False
command user_def_cmd3 :obj-user_def_cmd3-arg -> int
- gen=True success_response=True
+ gen=True success_response=True box=False
diff --git a/tests/qapi-schema/returns-int.out b/tests/qapi-schema/returns-int.out
index a2da259..911212d 100644
--- a/tests/qapi-schema/returns-int.out
+++ b/tests/qapi-schema/returns-int.out
@@ -1,3 +1,3 @@
object :empty
command guest-get-time None -> int
- gen=True success_response=True
+ gen=True success_response=True box=False
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 471f8e1..273cdc3 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -30,12 +30,14 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
print 'alternate %s' % name
self._print_variants(variants)
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response):
+ gen, success_response, box):
print 'command %s %s -> %s' % \
(name, arg_type and arg_type.name, ret_type and ret_type.name)
- print ' gen=%s success_response=%s' % (gen, success_response)
- def visit_event(self, name, info, arg_type):
+ print ' gen=%s success_response=%s box=%s' % (gen, success_response,
+ box)
+ def visit_event(self, name, info, arg_type, box):
print 'event %s %s' % (name, arg_type and arg_type.name)
+ print ' box=%s' % box
@staticmethod
def _print_variants(variants):
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 19/20] qapi: Implement boxed structs for commands/events
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (17 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 18/20] qapi: Plumb in 'box' to qapi generator lower levels Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 20/20] qapi: Support boxed unions Eric Blake
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Turn on the ability to pass command and event arguments in
a single boxed parameter. This patch merely tests the use
of the feature on structs. With this patch, we still reject
union types, and crash on { 'command':'foo', 'data': {
anonymous...}, 'box':true }; that will be addressed in the
next patch.
While this does not alter any clients, it opens the door for
clients to turn on boxing to receive a single pointer
parameter of the overall struct instead of a series of
parameters for each member of the struct; which will be
useful for commands whose argument struct has lots of members.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/qapi-code-gen.txt | 31 +++++++++++++++++++++++++------
scripts/qapi-commands.py | 14 +++++++++++---
scripts/qapi-event.py | 13 +++++++++++--
scripts/qapi.py | 11 ++++++++---
tests/qapi-schema/qapi-schema-test.json | 2 ++
tests/qapi-schema/qapi-schema-test.out | 4 ++++
tests/test-qmp-commands.c | 4 ++++
7 files changed, 65 insertions(+), 14 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 688a60c..668d2fc 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -397,7 +397,7 @@ following example objects:
=== Commands ===
Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
- '*returns': TYPE-NAME,
+ '*returns': TYPE-NAME, '*box': true,
'*gen': false, '*success-response': false }
Commands are defined by using a dictionary containing several members,
@@ -408,10 +408,9 @@ Client JSON Protocol command exchange.
The 'data' argument maps to the "arguments" dictionary passed in as
part of a Client JSON Protocol command. The 'data' member is optional
and defaults to {} (an empty dictionary). If present, it must be the
-string name of a complex type, a one-element array containing the name
-of a complex type, or a dictionary that declares an anonymous type
-with the same semantics as a 'struct' expression, with one exception
-noted below when 'gen' is used.
+string name of a complex type, or a dictionary that declares an
+anonymous type with the same semantics as a 'struct' expression, with
+one exception noted below when 'gen' is used.
The 'returns' member describes what will appear in the "return" field
of a Client JSON Protocol reply on successful completion of a command.
@@ -449,6 +448,17 @@ which would validate this Client JSON Protocol transaction:
=> { "execute": "my-second-command" }
<= { "return": [ { "value": "one" }, { } ] }
+By default, the generator creates a marshalling function that converts
+an input QDict into a function call implemented by the user, where the
+user's function protocol has a parameter for each member of the
+argument struct, including boolean arguments that describe whether
+optional arguments were provided. But if the QAPI description
+includes the key 'box' with the boolean value true, the user call will
+have only a single parameter for the overall generated C structure.
+The 'box' key is required in order to use a union as an input
+argument, since it is not possible to list all members of the union as
+separate parameters.
+
In rare cases, QAPI cannot express a type-safe representation of a
corresponding Client JSON Protocol command. You then have to suppress
generation of a marshalling function by including a key 'gen' with
@@ -472,7 +482,8 @@ use of this field.
=== Events ===
-Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT }
+Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
+ '*box': true }
Events are defined with the keyword 'event'. It is not allowed to
name an event 'MAX', since the generator also produces a C enumeration
@@ -493,6 +504,14 @@ Resulting in this JSON object:
"data": { "b": "test string" },
"timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
+By default, the generator creates a C function that takes as
+parameters each member of the argument struct and turns it into the
+appropriate JSON Client event. But if the QAPI description includes
+the key 'box' with the boolean value true, the event function will
+have only a single parameter for the overall generated C structure.
+The 'box' key is required in order to use a union as the data key,
+since it is not possible to list all members of the union as separate
+parameters.
== Code generation ==
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index e2b8f7a..d75a399 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -38,7 +38,7 @@ def gen_call(name, arg_type, box, ret_type):
argstr = ''
if box:
- assert False # not implemented
+ argstr = 'arg, '
else:
if arg_type:
for memb in arg_type.members:
@@ -87,7 +87,10 @@ Visitor *v;
''')
if box:
- assert False # not implemented
+ ret += mcgen('''
+%(c_type)s arg = %(c_null)s;
+''',
+ c_type=arg_type.c_type(), c_null=arg_type.c_null())
else:
for memb in arg_type.members:
if memb.optional:
@@ -135,7 +138,12 @@ v = qmp_input_get_visitor(mi);
''')
if box:
- assert False # not implemented
+ ret += mcgen('''
+visit_type_%(c_name)s(v, &arg, NULL, %(errp)s);
+
+''',
+ c_name=arg_type.c_name(), errp=errparg)
+ ret += gen_err_check(errarg)
else:
for memb in arg_type.members:
if memb.optional:
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 278cd7f..17f0c12 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -37,10 +37,13 @@ def gen_event_send(name, arg_type, box):
proto=gen_event_send_proto(name, arg_type, box))
if arg_type and arg_type.members:
+ if not box:
+ ret += mcgen('''
+ QObject *obj;
+''')
ret += mcgen('''
QmpOutputVisitor *qov;
Visitor *v;
- QObject *obj;
''')
@@ -66,7 +69,13 @@ def gen_event_send(name, arg_type, box):
''')
if box:
- assert False # not implemented
+ ret += mcgen('''
+ visit_type_%(c_name)s(v, &arg, NULL, &local_err);
+ if (local_err) {
+ goto clean;
+ }
+''',
+ c_name=arg_type.c_name(), name=arg_type.name)
else:
ret += mcgen('''
/* Fake visit, as if all members are under a structure */
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 313eaef..b907878 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -686,6 +686,10 @@ def check_keys(expr_elem, meta, required, optional=[]):
raise QAPIExprError(info,
"'%s' of %s '%s' should only use false value"
% (key, meta, name))
+ if key == 'box' and value != True:
+ raise QAPIExprError(info,
+ "'%s' of %s '%s' should only use true value"
+ % (key, meta, name))
for key in required:
if not expr.has_key(key):
raise QAPIExprError(info,
@@ -716,10 +720,10 @@ def check_exprs(exprs):
add_struct(expr, info)
elif expr.has_key('command'):
check_keys(expr_elem, 'command', [],
- ['data', 'returns', 'gen', 'success-response'])
+ ['data', 'returns', 'gen', 'success-response', 'box'])
add_name(expr['command'], info, 'command')
elif expr.has_key('event'):
- check_keys(expr_elem, 'event', [], ['data'])
+ check_keys(expr_elem, 'event', [], ['data', 'box'])
add_name(expr['event'], info, 'event')
else:
raise QAPIExprError(expr_elem['info'],
@@ -1426,7 +1430,8 @@ def gen_params(arg_type, box, extra):
ret = ''
sep = ''
if box:
- assert False # not implemented
+ ret += '%s arg' % arg_type.c_type(is_param=True)
+ sep = ', '
else:
assert not arg_type.variants
for memb in arg_type.members:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 45143e3..4acb57a 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -93,6 +93,7 @@
'returns': 'int' }
# note: command name 'guest-sync' chosen to avoid "cannot use built-in" error
{ 'command': 'guest-sync', 'data': { 'arg': 'any' }, 'returns': 'any' }
+{ 'command': 'boxed', 'box': true, 'data': 'UserDefZero' }
# For testing integer range flattening in opts-visitor. The following schema
# corresponds to the option format:
@@ -120,6 +121,7 @@
'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } }
{ 'event': 'EVENT_D',
'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } }
+{ 'event': 'EVENT_E', 'box': true, 'data': 'UserDefZero' }
# test that we correctly compile downstream extensions
{ 'enum': '__org.qemu_x-Enum', 'data': [ '__org.qemu_x-value' ] }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 1a66627..8b120ee 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -77,6 +77,8 @@ event EVENT_C :obj-EVENT_C-arg
box=False
event EVENT_D :obj-EVENT_D-arg
box=False
+event EVENT_E UserDefZero
+ box=True
enum EnumOne ['value1', 'value2', 'value3']
object EventStructOne
member struct1: UserDefOne optional=False
@@ -173,6 +175,8 @@ object __org.qemu_x-Union2
case __org.qemu_x-value: __org.qemu_x-Struct2
command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> __org.qemu_x-Union1
gen=True success_response=True box=False
+command boxed UserDefZero -> None
+ gen=True success_response=True box=True
command guest-sync :obj-guest-sync-arg -> any
gen=True success_response=True box=False
command user_def_cmd None -> None
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 89a3b47..a190f31 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -54,6 +54,10 @@ QObject *qmp_guest_sync(QObject *arg, Error **errp)
return arg;
}
+void qmp_boxed(UserDefZero *arg, Error **errp)
+{
+}
+
__org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
__org_qemu_x_StructList *b,
__org_qemu_x_Union2 *c,
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH RFC v3 20/20] qapi: Support boxed unions
2015-08-18 14:19 [Qemu-devel] [PATCH RFC v3 00/20] post-introspection qapi cleanups Eric Blake
` (18 preceding siblings ...)
2015-08-18 16:05 ` [Qemu-devel] [PATCH RFC v3 19/20] qapi: Implement boxed structs for commands/events Eric Blake
@ 2015-08-18 16:05 ` Eric Blake
19 siblings, 0 replies; 21+ messages in thread
From: Eric Blake @ 2015-08-18 16:05 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
This patch completes support for boxed types, by allowing
union types to be used when 'box':true is specified. It also
avoids a python crash when attempting to use boxing on an
anonymous type. While it was possible to support 'box':true
on an empty event, it was easier to just reject missing 'data'
for commands. Add some tests to cover new error messages, as
well as enhancing qapi-schema-test to cover situations that
are now valid.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi.py | 20 ++++++++++++++++----
tests/Makefile | 4 ++--
tests/qapi-schema/args-bad-box.err | 1 +
tests/qapi-schema/args-bad-box.exit | 1 +
tests/qapi-schema/args-bad-box.json | 2 ++
tests/qapi-schema/args-bad-box.out | 0
tests/qapi-schema/args-box-anon.err | 1 +
tests/qapi-schema/args-box-anon.exit | 1 +
tests/qapi-schema/args-box-anon.json | 2 ++
tests/qapi-schema/args-box-anon.out | 0
tests/qapi-schema/args-box-empty.err | 1 +
tests/qapi-schema/args-box-empty.exit | 1 +
tests/qapi-schema/args-box-empty.json | 2 ++
tests/qapi-schema/args-box-empty.out | 0
tests/qapi-schema/args-union.err | 2 +-
tests/qapi-schema/args-union.json | 3 +--
tests/qapi-schema/qapi-schema-test.json | 3 ++-
tests/qapi-schema/qapi-schema-test.out | 4 +++-
tests/test-qmp-commands.c | 4 ++++
19 files changed, 41 insertions(+), 11 deletions(-)
create mode 100644 tests/qapi-schema/args-bad-box.err
create mode 100644 tests/qapi-schema/args-bad-box.exit
create mode 100644 tests/qapi-schema/args-bad-box.json
create mode 100644 tests/qapi-schema/args-bad-box.out
create mode 100644 tests/qapi-schema/args-box-anon.err
create mode 100644 tests/qapi-schema/args-box-anon.exit
create mode 100644 tests/qapi-schema/args-box-anon.json
create mode 100644 tests/qapi-schema/args-box-anon.out
create mode 100644 tests/qapi-schema/args-box-empty.err
create mode 100644 tests/qapi-schema/args-box-empty.exit
create mode 100644 tests/qapi-schema/args-box-empty.json
create mode 100644 tests/qapi-schema/args-box-empty.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index b907878..1e73500 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -491,10 +491,18 @@ def check_member_clash(expr_info, base_name, data, source = ""):
def check_command(expr, expr_info):
name = expr['command']
+ box = expr.get('box', False)
+ args_meta = ['struct']
+ if box:
+ args_meta += ['union']
+ if not expr.get('data'):
+ raise QAPIExprError(expr_info,
+ "Use of 'box' requires 'data' for command '%s'"
+ % name)
check_type(expr_info, "'data' for command '%s'" % name,
- expr.get('data'), allow_dict=True, allow_optional=True,
- allow_metas=['struct'])
+ expr.get('data'), allow_dict=not box, allow_optional=True,
+ allow_metas=args_meta)
returns_meta = ['union', 'struct']
if name in returns_whitelist:
returns_meta += ['built-in', 'alternate', 'enum']
@@ -505,13 +513,17 @@ def check_command(expr, expr_info):
def check_event(expr, expr_info):
global events
name = expr['event']
+ box = expr.get('box', False)
+ meta = ['struct']
+ if box:
+ meta += ['union']
if name.upper() == 'MAX':
raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created")
events.append(name)
check_type(expr_info, "'data' for event '%s'" % name,
- expr.get('data'), allow_dict=True, allow_optional=True,
- allow_metas=['struct'])
+ expr.get('data'), allow_dict=not box, allow_optional=True,
+ allow_metas=meta)
def check_union(expr, expr_info):
name = expr['union']
diff --git a/tests/Makefile b/tests/Makefile
index 8838f11..33578ea 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -228,11 +228,11 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
bad-type-dict.json double-data.json unknown-expr-key.json \
redefined-type.json redefined-command.json redefined-builtin.json \
redefined-event.json command-int.json bad-data.json event-max.json \
- type-bypass-bad-gen.json \
+ type-bypass-bad-gen.json args-bad-box.json \
args-array-empty.json args-array-unknown.json args-int.json \
args-unknown.json args-member-unknown.json args-member-array.json \
args-member-array-bad.json args-alternate.json args-union.json \
- args-any.json \
+ args-any.json args-box-anon.json args-box-empty.json \
returns-array-bad.json returns-int.json returns-dict.json \
returns-unknown.json returns-alternate.json returns-whitelist.json \
missing-colon.json missing-comma-list.json missing-comma-object.json \
diff --git a/tests/qapi-schema/args-bad-box.err b/tests/qapi-schema/args-bad-box.err
new file mode 100644
index 0000000..16afe3c
--- /dev/null
+++ b/tests/qapi-schema/args-bad-box.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-bad-box.json:2: 'box' of command 'foo' should only use true value
diff --git a/tests/qapi-schema/args-bad-box.exit b/tests/qapi-schema/args-bad-box.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-bad-box.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-bad-box.json b/tests/qapi-schema/args-bad-box.json
new file mode 100644
index 0000000..8d5737a
--- /dev/null
+++ b/tests/qapi-schema/args-bad-box.json
@@ -0,0 +1,2 @@
+# 'box' should only appear with value true
+{ 'command': 'foo', 'box': false }
diff --git a/tests/qapi-schema/args-bad-box.out b/tests/qapi-schema/args-bad-box.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-box-anon.err b/tests/qapi-schema/args-box-anon.err
new file mode 100644
index 0000000..11eaefc
--- /dev/null
+++ b/tests/qapi-schema/args-box-anon.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-box-anon.json:2: 'data' for command 'foo' should be a type name
diff --git a/tests/qapi-schema/args-box-anon.exit b/tests/qapi-schema/args-box-anon.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-box-anon.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-box-anon.json b/tests/qapi-schema/args-box-anon.json
new file mode 100644
index 0000000..947e3c6
--- /dev/null
+++ b/tests/qapi-schema/args-box-anon.json
@@ -0,0 +1,2 @@
+# 'box' can only be used with named types
+{ 'command': 'foo', 'box': true, 'data': { 'string': 'str' } }
diff --git a/tests/qapi-schema/args-box-anon.out b/tests/qapi-schema/args-box-anon.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-box-empty.err b/tests/qapi-schema/args-box-empty.err
new file mode 100644
index 0000000..574c8b5
--- /dev/null
+++ b/tests/qapi-schema/args-box-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-box-empty.json:2: Use of 'box' requires 'data' for command 'foo'
diff --git a/tests/qapi-schema/args-box-empty.exit b/tests/qapi-schema/args-box-empty.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-box-empty.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-box-empty.json b/tests/qapi-schema/args-box-empty.json
new file mode 100644
index 0000000..31dca5e
--- /dev/null
+++ b/tests/qapi-schema/args-box-empty.json
@@ -0,0 +1,2 @@
+# 'box' can only be used with named types
+{ 'command': 'foo', 'box': true }
diff --git a/tests/qapi-schema/args-box-empty.out b/tests/qapi-schema/args-box-empty.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-union.err b/tests/qapi-schema/args-union.err
index 1d693d7..f8ad223 100644
--- a/tests/qapi-schema/args-union.err
+++ b/tests/qapi-schema/args-union.err
@@ -1 +1 @@
-tests/qapi-schema/args-union.json:4: 'data' for command 'oops' cannot use union type 'Uni'
+tests/qapi-schema/args-union.json:3: 'data' for command 'oops' cannot use union type 'Uni'
diff --git a/tests/qapi-schema/args-union.json b/tests/qapi-schema/args-union.json
index 7bdcbb7..c0ce091 100644
--- a/tests/qapi-schema/args-union.json
+++ b/tests/qapi-schema/args-union.json
@@ -1,4 +1,3 @@
-# we do not allow union arguments
-# TODO should we support this?
+# use of union arguments requires 'box':true
{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
{ 'command': 'oops', 'data': 'Uni' }
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 4acb57a..b4893ce 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -94,6 +94,7 @@
# note: command name 'guest-sync' chosen to avoid "cannot use built-in" error
{ 'command': 'guest-sync', 'data': { 'arg': 'any' }, 'returns': 'any' }
{ 'command': 'boxed', 'box': true, 'data': 'UserDefZero' }
+{ 'command': 'boxed2', 'data': 'UserDefNativeListUnion', 'box': true }
# For testing integer range flattening in opts-visitor. The following schema
# corresponds to the option format:
@@ -114,7 +115,7 @@
{ 'struct': 'EventStructOne',
'data': { 'struct1': 'UserDefOne', 'string': 'str', '*enum2': 'EnumOne' } }
-{ 'event': 'EVENT_A' }
+{ 'event': 'EVENT_A', 'box': true }
{ 'event': 'EVENT_B',
'data': { } }
{ 'event': 'EVENT_C',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8b120ee..89f50b8 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -70,7 +70,7 @@ alternate AltTwo
case s: str
case n: number
event EVENT_A None
- box=False
+ box=True
event EVENT_B None
box=False
event EVENT_C :obj-EVENT_C-arg
@@ -177,6 +177,8 @@ command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> __org.qemu_x-Union
gen=True success_response=True box=False
command boxed UserDefZero -> None
gen=True success_response=True box=True
+command boxed2 UserDefNativeListUnion -> None
+ gen=True success_response=True box=True
command guest-sync :obj-guest-sync-arg -> any
gen=True success_response=True box=False
command user_def_cmd None -> None
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index a190f31..886de98 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -58,6 +58,10 @@ void qmp_boxed(UserDefZero *arg, Error **errp)
{
}
+void qmp_boxed2(UserDefNativeListUnion *arg, Error **errp)
+{
+}
+
__org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
__org_qemu_x_StructList *b,
__org_qemu_x_Union2 *c,
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread