| Message ID | 1486569864-17005-2-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/08/2017 10:04 AM, Markus Armbruster wrote: > Simple unions are simpler than flat unions in the schema, but more > complicated in C and on the QMP wire: there's extra indirection in C > and extra nesting on the wire, both pointless. They're best avoided > in new code. > > NumaOptions isn't new, but it's only used internally, not in QMP. > Convert it to a flat union. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > numa.c | 4 ++-- > qapi-schema.json | 8 ++++++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > +++ b/qapi-schema.json > @@ -5545,6 +5545,12 @@ > 'events' : [ 'InputEvent' ] } } > > ## > +# @NumaOptionsType: > +## Do we want a 'Since: 2.1' designation (since the enum has effectively existed, even if implicitly, for as long as NumaOptions has been around)? With or without that extra line, Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 02/08/2017 10:04 AM, Markus Armbruster wrote: >> Simple unions are simpler than flat unions in the schema, but more >> complicated in C and on the QMP wire: there's extra indirection in C >> and extra nesting on the wire, both pointless. They're best avoided >> in new code. >> >> NumaOptions isn't new, but it's only used internally, not in QMP. >> Convert it to a flat union. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> numa.c | 4 ++-- >> qapi-schema.json | 8 ++++++++ >> 2 files changed, 10 insertions(+), 2 deletions(-) >> > >> +++ b/qapi-schema.json >> @@ -5545,6 +5545,12 @@ >> 'events' : [ 'InputEvent' ] } } >> >> ## >> +# @NumaOptionsType: >> +## > > Do we want a 'Since: 2.1' designation (since the enum has effectively > existed, even if implicitly, for as long as NumaOptions has been around)? Version tracking is useful only for things visible external interfaces, which this is not. Tool support to require it for external and reject it for internal stuff would be nice. Until then, we need to rely on review to get external stuff tracked properly. Tracking internal stuff is busywork. If we think it's less work than deciding internal vs. external, we can slap on "Since:" blindly. > With or without that extra line, > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
diff --git a/numa.c b/numa.c index 9f56be9..e01cb54 100644 --- a/numa.c +++ b/numa.c @@ -228,8 +228,8 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp) } switch (object->type) { - case NUMA_OPTIONS_KIND_NODE: - numa_node_parse(object->u.node.data, opts, &err); + case NUMA_OPTIONS_TYPE_NODE: + numa_node_parse(&object->u.node, opts, &err); if (err) { goto end; } diff --git a/qapi-schema.json b/qapi-schema.json index cbdffdd..f9a9941 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -5545,6 +5545,12 @@ 'events' : [ 'InputEvent' ] } } ## +# @NumaOptionsType: +## +{ 'enum': 'NumaOptionsType', + 'data': [ 'node' ] } + +## # @NumaOptions: # # A discriminated record of NUMA options. (for OptsVisitor) @@ -5552,6 +5558,8 @@ # Since: 2.1 ## { 'union': 'NumaOptions', + 'base': { 'type': 'NumaOptionsType' }, + 'discriminator': 'type', 'data': { 'node': 'NumaNodeOptions' }}
Simple unions are simpler than flat unions in the schema, but more complicated in C and on the QMP wire: there's extra indirection in C and extra nesting on the wire, both pointless. They're best avoided in new code. NumaOptions isn't new, but it's only used internally, not in QMP. Convert it to a flat union. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- numa.c | 4 ++-- qapi-schema.json | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-)