* [Qemu-devel] [RFC PATCH 1/2] vnc: hoist allocation of VncBasicInfo to callers
2015-07-27 21:28 [Qemu-devel] [RFC PATCH 0/2] qapi: unbox base classes Eric Blake
@ 2015-07-27 21:28 ` Eric Blake
2015-07-27 21:28 ` [Qemu-devel] [RFC PATCH 2/2] qapi: unbox base members Eric Blake
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2015-07-27 21:28 UTC (permalink / raw)
To: qemu-devel; +Cc: 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 1483958..7e2a017 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] 7+ messages in thread* [Qemu-devel] [RFC PATCH 2/2] qapi: unbox base members
2015-07-27 21:28 [Qemu-devel] [RFC PATCH 0/2] qapi: unbox base classes Eric Blake
2015-07-27 21:28 ` [Qemu-devel] [RFC PATCH 1/2] vnc: hoist allocation of VncBasicInfo to callers Eric Blake
@ 2015-07-27 21:28 ` Eric Blake
2015-07-29 16:54 ` [Qemu-devel] [RFC PATCH 3/2] qapi-visit: Remove redundant functions for flat union base Eric Blake
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2015-07-27 21:28 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, Gerd Hoffmann, Markus Armbruster, 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.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Note that there are probably further cleanups possible (for example,
smarter ways of generating a forward declaration of visit_FOO_fields
only when needed, while still maintaining alphabetical sorting of
the overall file; rather than my first cut which results in topological
rather than alphabetical sorting in some cases). Likewise, it will
probably need a rebase on top of Markus' series once that is no
longer RFC. But the idea of posting this now is to get a feel for
what this does to our code base.
hmp.c | 6 +++---
scripts/qapi-types.py | 13 ++++++++++---
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, 60 insertions(+), 64 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 c1db5e2..f04b6cc 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -47,11 +47,18 @@ 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=None):
ret = ''
+ if base and base.base:
+ ret += gen_struct_fields(base.base.local_members, base.base)
for memb in members:
ret += gen_struct_field(memb.name, memb.type, memb.optional)
+ if base:
+ ret += mcgen('''
+ /* End fields inherited from %(c_name)s. */
+''',
+ c_name=base.c_name())
return ret
def gen_struct(name, base, members):
@@ -62,7 +69,7 @@ struct %(c_name)s {
c_name=c_name(name))
if base:
- ret += gen_struct_field('base', base, False)
+ ret += gen_struct_fields(base.local_members, base)
ret += gen_struct_fields(members)
@@ -117,7 +124,7 @@ struct %(c_name)s {
''',
c_name=c_name(name))
if base:
- ret += gen_struct_fields(base.members)
+ ret += gen_struct_fields(base.local_members, base)
ret += mcgen('''
/* union tag is %(c_type)s %(c_name)s */
''',
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index f7faa22..30bd471 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 87a6c30..8350b29 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");
@@ -171,20 +169,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 8662a57..2360438 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -265,7 +265,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);
@@ -295,7 +295,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 7e2a017..5b5b936 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] 7+ messages in thread* [Qemu-devel] [RFC PATCH 3/2] qapi-visit: Remove redundant functions for flat union base
2015-07-27 21:28 [Qemu-devel] [RFC PATCH 0/2] qapi: unbox base classes Eric Blake
2015-07-27 21:28 ` [Qemu-devel] [RFC PATCH 1/2] vnc: hoist allocation of VncBasicInfo to callers Eric Blake
2015-07-27 21:28 ` [Qemu-devel] [RFC PATCH 2/2] qapi: unbox base members Eric Blake
@ 2015-07-29 16:54 ` Eric Blake
2015-07-30 21:48 ` [Qemu-devel] [RFC PATCH 4/2] qapi: Test use of 'number' within alternates Eric Blake
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2015-07-29 16:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, 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.
The function signature of gen_visit_struct_fields() is changed
to operate on the actual type object, which in turn requires
some tweaks to the visitor to supply the type object directly;
maybe this implies that the visitor interface should be changed
instead.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Solves the concern mentioned here:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05596.html
scripts/qapi-visit.py | 61 +++++++++++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 30bd471..728abae 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -58,16 +58,15 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
c_type=typ.c_name())
return ret
-def gen_visit_struct_fields(name, base, members):
- if name in struct_fields_seen:
+def gen_visit_struct_fields(typ):
+ if typ.name in struct_fields_seen:
return ''
- struct_fields_seen.add(name)
+ struct_fields_seen.add(typ.name)
ret = ''
- if base:
- ret += gen_visit_struct_fields(base.name, base.base,
- base.local_members)
+ if typ.base:
+ ret += gen_visit_struct_fields(typ.base)
ret += mcgen('''
@@ -76,19 +75,19 @@ static void visit_type_%(c_name)s_fields(Visitor *m, %(c_name)s **obj, Error **e
Error *err = NULL;
''',
- c_name=c_name(name))
+ c_name=c_name(typ.name))
push_indent()
- if base:
+ if typ.base:
ret += mcgen('''
visit_type_%(c_type)s_fields(m, (%(c_type)s **)obj, &err);
if (err) {
goto out;
}
''',
- c_type=base.c_name())
+ c_type=typ.base.c_name())
- for memb in members:
+ for memb in typ.local_members:
if memb.optional:
ret += mcgen('''
visit_optional(m, &(*obj)->has_%(c_name)s, "%(name)s", &err);
@@ -115,7 +114,7 @@ if (err) {
''')
pop_indent()
- if re.search('^ *goto out;', ret, re.MULTILINE):
+ if typ.base or typ.local_members:
ret += mcgen('''
out:
@@ -126,8 +125,8 @@ out:
''')
return ret
-def gen_visit_struct(name, base, members):
- ret = gen_visit_struct_fields(name, base, members)
+def gen_visit_struct(name, base, members, schema):
+ ret = gen_visit_struct_fields(schema.lookup_type(name))
ret += mcgen('''
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
@@ -236,8 +235,7 @@ 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)
for var in variants.variants:
if var.flat:
@@ -257,29 +255,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:
ret += mcgen('''
@@ -326,10 +326,12 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
self.decl = None
self.defn = None
self.btin = None
+ self.schema = None
def visit_begin(self, schema):
self.decl = ''
self.defn = ''
self.btin = guardstart('QAPI_VISIT_BUILTIN_VISITOR_DECL')
+ self.schema = schema
def visit_end(self):
# to avoid header dependency hell, we always generate
# declarations for built-in types in our header files and
@@ -340,6 +342,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
# ...this doesn't work for cases where we link in multiple
# objects that have the functions defined, so we use
# do_builtins (option -b) to provide control
+ self.schema = None
def visit_enum_type(self, name, info, values):
self.decl += gen_visit_decl(name, scalar=True)
self.defn += gen_visit_enum(name)
@@ -359,7 +362,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
if variants:
self.defn += gen_visit_union(name, base, variants)
else:
- self.defn += gen_visit_struct(name, base, members)
+ self.defn += gen_visit_struct(name, base, members, self.schema)
def visit_alternate_type(self, name, info, variants):
self.decl += gen_visit_decl(name)
self.defn += gen_visit_alternate(name, variants)
--
2.4.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [RFC PATCH 4/2] qapi: Test use of 'number' within alternates
2015-07-27 21:28 [Qemu-devel] [RFC PATCH 0/2] qapi: unbox base classes Eric Blake
` (2 preceding siblings ...)
2015-07-29 16:54 ` [Qemu-devel] [RFC PATCH 3/2] qapi-visit: Remove redundant functions for flat union base Eric Blake
@ 2015-07-30 21:48 ` Eric Blake
2015-07-30 21:48 ` [Qemu-devel] [RFC PATCH 5/2] qapi: Simplify visiting of alternate types Eric Blake
2015-07-30 21:48 ` [Qemu-devel] [RFC PATCH 6/2] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
5 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2015-07-30 21:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, 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 suggested here:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05944.html
tests/qapi-schema/qapi-schema-test.json | 8 ++
tests/qapi-schema/qapi-schema-test.out | 23 ++++++
tests/test-qmp-input-visitor.c | 129 +++++++++++++++++++++++++++++++-
3 files changed, 157 insertions(+), 3 deletions(-)
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 90b4740..b28d833 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' } }
+{ '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 8a12d51..9d52ef3 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -21,6 +21,29 @@ object :obj-user_def_cmd2-args
object :obj-user_def_cmd3-args
member a: int optional=False
member b: int optional=True
+alternate AltFive
+ case i: int flat=False
+ case n: number flat=False
+enum AltFiveKind ['i', 'n']
+alternate AltFour
+ case s: str flat=False
+ case i: int flat=False
+enum AltFourKind ['s', 'i']
+alternate AltOne
+ case s: str flat=False
+enum AltOneKind ['s']
+alternate AltSix
+ case n: number flat=False
+ case i: int flat=False
+enum AltSixKind ['n', 'i']
+alternate AltThree
+ case n: number flat=False
+ case s: str flat=False
+enum AltThreeKind ['n', 's']
+alternate AltTwo
+ case s: str flat=False
+ case n: number flat=False
+enum AltTwoKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-data
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 2360438..56feedf 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -371,15 +371,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,
@@ -746,6 +867,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] 7+ messages in thread* [Qemu-devel] [RFC PATCH 5/2] qapi: Simplify visiting of alternate types
2015-07-27 21:28 [Qemu-devel] [RFC PATCH 0/2] qapi: unbox base classes Eric Blake
` (3 preceding siblings ...)
2015-07-30 21:48 ` [Qemu-devel] [RFC PATCH 4/2] qapi: Test use of 'number' within alternates Eric Blake
@ 2015-07-30 21:48 ` Eric Blake
2015-07-30 21:48 ` [Qemu-devel] [RFC PATCH 6/2] qapi: Fix alternates that accept 'number' but not 'int' Eric Blake
5 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2015-07-30 21:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, 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>
---
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 | 36 ++++------------------------------
scripts/qapi-visit.py | 12 +++++++-----
scripts/qapi.py | 25 ++++++++++++++++-------
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 +++++++++++++++-----
11 files changed, 64 insertions(+), 77 deletions(-)
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 230d4b2..643e36b 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 f04b6cc..fd56659 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -87,36 +87,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('''
@@ -130,6 +100,10 @@ struct %(c_name)s {
''',
c_type=c_name(variants.tag_member.type.name),
c_name=c_name(variants.tag_member.name))
+ elif not variants.tag_member:
+ ret += mcgen('''
+ qtype_code type;
+''')
else:
ret += mcgen('''
%(c_type)s type;
@@ -238,9 +212,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)
do_builtins = False
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 728abae..2907ce8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -197,7 +197,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;
}
@@ -211,14 +211,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);
@@ -227,7 +227,8 @@ out_end:
out:
error_propagate(errp, err);
}
-''')
+''',
+ name=name)
return ret
@@ -417,6 +418,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 5fedfee..9e4e44a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -920,22 +920,27 @@ 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, flat):
@@ -944,7 +949,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
self.flat = flat
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
if self.flat:
self.type.check(schema)
assert isinstance(self.type, QAPISchemaObjectType)
@@ -1115,6 +1121,11 @@ class QAPISchema(object):
[v.name for v in variants])
return QAPISchemaObjectTypeVariants(None, enum, variants)
+ def _make_alternate_variants(self, data):
+ variants = [self._make_simple_variant(key, data[key])
+ for key in data.keys()]
+ return QAPISchemaObjectTypeVariants(None, None, variants)
+
def _def_union_type(self, expr, info):
name = expr['union']
data = expr['data']
@@ -1134,7 +1145,7 @@ class QAPISchema(object):
name = expr['alternate']
data = expr['data']
self._def_entity(QAPISchemaAlternateType(name, info,
- self._make_simple_variants(name, data)))
+ self._make_alternate_variants(data)))
self._make_array_type(name) # TODO really needed?
def _def_command(self, expr, info):
diff --git a/tests/qapi-schema/alternate-good.out b/tests/qapi-schema/alternate-good.out
index aede1ae..e2d26b0 100644
--- a/tests/qapi-schema/alternate-good.out
+++ b/tests/qapi-schema/alternate-good.out
@@ -3,7 +3,6 @@ alternate Alt
case value: int flat=False
case string: Enum flat=False
case struct: Data flat=False
-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 9d52ef3..1585bad 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -24,26 +24,20 @@ object :obj-user_def_cmd3-args
alternate AltFive
case i: int flat=False
case n: number flat=False
-enum AltFiveKind ['i', 'n']
alternate AltFour
case s: str flat=False
case i: int flat=False
-enum AltFourKind ['s', 'i']
alternate AltOne
case s: str flat=False
-enum AltOneKind ['s']
alternate AltSix
case n: number flat=False
case i: int flat=False
-enum AltSixKind ['n', 'i']
alternate AltThree
case n: number flat=False
case s: str flat=False
-enum AltThreeKind ['n', 's']
alternate AltTwo
case s: str flat=False
case n: number flat=False
-enum AltTwoKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-data
@@ -64,7 +58,6 @@ alternate UserDefAlternate
case uda: UserDefA flat=False
case s: str flat=False
case i: int flat=False
-enum UserDefAlternateKind ['uda', 's', 'i']
object UserDefB
member intb: int optional=False
object UserDefC
@@ -127,7 +120,6 @@ event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
alternate __org.qemu_x-Alt
case __org.qemu_x-branch: str flat=False
case b: __org.qemu_x-Base flat=False
-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 56feedf..f89ede3 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -376,7 +376,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;
@@ -384,7 +384,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;
@@ -427,31 +427,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;
@@ -468,14 +468,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;
@@ -490,14 +490,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] 7+ messages in thread* [Qemu-devel] [RFC PATCH 6/2] qapi: Fix alternates that accept 'number' but not 'int'
2015-07-27 21:28 [Qemu-devel] [RFC PATCH 0/2] qapi: unbox base classes Eric Blake
` (4 preceding siblings ...)
2015-07-30 21:48 ` [Qemu-devel] [RFC PATCH 5/2] qapi: Simplify visiting of alternate types Eric Blake
@ 2015-07-30 21:48 ` Eric Blake
5 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2015-07-30 21:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, 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 643e36b..f53639f 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 2907ce8..679515f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -187,6 +187,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)
@@ -197,13 +202,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 f89ede3..308916a 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -418,20 +418,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] 7+ messages in thread