Commit b5bff751 authored by Peter Maydell's avatar Peter Maydell
Browse files

Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-09-04' into staging



qapi: Another round of fixes and cleanups

# gpg: Signature made Fri 04 Sep 2015 14:48:54 BST using RSA key ID EB918653
# gpg: Good signature from "Markus Armbruster <armbru@redhat.com>"
# gpg:                 aka "Markus Armbruster <armbru@pond.sub.org>"

* remotes/armbru/tags/pull-qapi-2015-09-04: (33 commits)
  qapi: Generators crash when --output-dir isn't given, fix
  docs/qapi-code-gen.txt: Fix QAPI schema examples
  qapi: Simplify error reporting for array types
  qapi: Fix errors for non-string, non-dictionary members
  tests/qapi-schema: Cover non-string, non-dictionary members
  tests/qapi-schema: Cover two more syntax errors
  qapi: Drop one of two "simple union must not have base" checks
  qapi: Generated code cleanup
  qapi-commands: Drop useless initialization
  qapi-commands: Don't feed output of mcgen() to mcgen() again
  qapi-commands: Inline gen_marshal_output_call()
  qapi-commands: Fix gen_err_check(e) for e and e != 'local_err'
  qapi: Command returning anonymous type doesn't work, outlaw
  qapi: Fix to reject union command and event arguments
  qapi-tests: New tests for union, alternate command arguments
  tests/qapi-schema: Rename tests from data- to args-
  tests/qapi-schema: Restore test case for flat union base bug
  qapi: Document flaws in checking of names
  qapi: Document shortcoming with union 'data' branch
  qapi: Document that input visitor semantics are prone to leaks
  ...

Signed-off-by: default avatarPeter Maydell <peter.maydell@linaro.org>
parents b0410664 c4f498fe
Loading
Loading
Loading
Loading
+24 −29
Original line number Diff line number Diff line
@@ -163,7 +163,7 @@ The QAPI schema definitions can be modularized using the 'include' directive:

The directive is evaluated recursively, and include paths are relative to the
file using the directive. Multiple includes of the same file are
safe.  No other keys should appear in the expression, and the include
idempotent.  No other keys should appear in the expression, and the include
value should be a string.

As a matter of style, it is a good idea to have all files be
@@ -300,7 +300,6 @@ an implicit C enum 'NameKind' is created, corresponding to the union
the union can be named 'max', as this would collide with the implicit
enum.  The value for each branch can be of any type.


A flat union definition specifies a struct as its base, and
avoids nesting on the wire.  All branches of the union must be
complex types, and the top-level fields of the union dictionary on
@@ -314,7 +313,7 @@ adding a common field 'readonly', renaming the discriminator to
something more applicable, and reducing the number of {} required on
the wire:

 { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
 { 'enum': 'BlockdevDriver', 'data': [ 'file', 'qcow2' ] }
 { 'struct': 'BlockdevCommonOptions',
   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
 { 'union': 'BlockdevOptions',
@@ -350,7 +349,7 @@ is identical on the wire to:
 { 'struct': 'Base', 'data': { 'type': 'Enum' } }
 { 'struct': 'Branch1', 'data': { 'data': 'str' } }
 { 'struct': 'Branch2', 'data': { 'data': 'int' } }
 { 'union': 'Flat': 'base': 'Base', 'discriminator': 'type',
 { 'union': 'Flat', 'base': 'Base', 'discriminator': 'type',
   'data': { 'one': 'Branch1', 'two': 'Branch2' } }


@@ -394,7 +393,7 @@ following example objects:
=== Commands ===

Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
         '*returns': TYPE-NAME-OR-DICT,
         '*returns': TYPE-NAME,
         '*gen': false, '*success-response': false }

Commands are defined by using a dictionary containing several members,
@@ -405,10 +404,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.
@@ -416,14 +414,13 @@ The member is optional from the command declaration; if absent, the
"return" field will be an empty dictionary.  If 'returns' is present,
it must be the string name of a complex or built-in type, a
one-element array containing the name of a complex or built-in 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.  Although it is permitted to have the 'returns'
member name a built-in type or an array of built-in types, any command
that does this cannot be extended to return additional information in
the future; thus, new commands should strongly consider returning a
dictionary-based type or an array of dictionaries, even if the
dictionary only contains one field at the present.
with one exception noted below when 'gen' is used.  Although it is
permitted to have the 'returns' member name a built-in type or an
array of built-in types, any command that does this cannot be extended
to return additional information in the future; thus, new commands
should strongly consider returning a dictionary-based type or an array
of dictionaries, even if the dictionary only contains one field at the
present.

All commands in Client JSON Protocol use a dictionary to report
failure, with no way to specify that in QAPI.  Where the error return
@@ -555,6 +552,7 @@ Example:
        qapi_dealloc_visitor_cleanup(md);
    }


    void qapi_free_UserDefOne(UserDefOne *obj)
    {
        QapiDeallocVisitor *md;
@@ -569,7 +567,6 @@ Example:
        visit_type_UserDefOne(v, &obj, NULL, NULL);
        qapi_dealloc_visitor_cleanup(md);
    }

    $ cat qapi-generated/example-qapi-types.h
[Uninteresting stuff omitted...]

@@ -580,8 +577,7 @@ Example:

    typedef struct UserDefOne UserDefOne;

    typedef struct UserDefOneList
    {
    typedef struct UserDefOneList {
        union {
            UserDefOne *value;
            uint64_t padding;
@@ -589,10 +585,10 @@ Example:
        struct UserDefOneList *next;
    } UserDefOneList;


[Functions on built-in types omitted...]

    struct UserDefOne
    {
    struct UserDefOne {
        int64_t integer;
        char *string;
    };
@@ -630,6 +626,7 @@ Example:
    static void visit_type_UserDefOne_fields(Visitor *m, UserDefOne **obj, Error **errp)
    {
        Error *err = NULL;

        visit_type_int(m, &(*obj)->integer, "integer", &err);
        if (err) {
            goto out;
@@ -743,7 +740,7 @@ Example:
    static void qmp_marshal_input_my_command(QDict *args, QObject **ret, Error **errp)
    {
        Error *local_err = NULL;
        UserDefOne *retval = NULL;
        UserDefOne *retval;
        QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
        QapiDeallocVisitor *md;
        Visitor *v;
@@ -769,7 +766,6 @@ Example:
        v = qapi_dealloc_get_visitor(md);
        visit_type_UserDefOne(v, &arg1, "arg1", NULL);
        qapi_dealloc_visitor_cleanup(md);
        return;
    }

    static void qmp_init_marshal(void)
@@ -826,7 +822,7 @@ Example:
        QDECREF(qmp);
    }

    const char *EXAMPLE_QAPIEvent_lookup[] = {
    const char *example_QAPIEvent_lookup[] = {
        "MY_EVENT",
        NULL,
    };
@@ -843,11 +839,10 @@ Example:

    void qapi_event_send_my_event(Error **errp);

    extern const char *EXAMPLE_QAPIEvent_lookup[];
    typedef enum EXAMPLE_QAPIEvent
    {
    extern const char *example_QAPIEvent_lookup[];
    typedef enum example_QAPIEvent {
        EXAMPLE_QAPI_EVENT_MY_EVENT = 0,
        EXAMPLE_QAPI_EVENT_MAX = 1,
    } EXAMPLE_QAPIEvent;
    } example_QAPIEvent;

    #endif
+46 −63
Original line number Diff line number Diff line
@@ -27,18 +27,19 @@ def generate_command_decl(name, args, ret_type):
%(ret_type)s qmp_%(name)s(%(args)sError **errp);
''',
                 ret_type=c_type(ret_type), name=c_name(name),
                 args=arglist).strip()
                 args=arglist)

def gen_err_check(errvar):
    if errvar:
def gen_err_check(err):
    if not err:
        return ''
    return mcgen('''
if (local_err) {
if (%(err)s) {
    goto out;
}
''')
    return ''
''',
                 err=err)

def gen_sync_call(name, args, ret_type, indent=0):
def gen_sync_call(name, args, ret_type):
    ret = ""
    arglist=""
    retval=""
@@ -48,41 +49,34 @@ def gen_sync_call(name, args, ret_type, indent=0):
        if optional:
            arglist += "has_%s, " % c_name(argname)
        arglist += "%s, " % (c_name(argname))
    push_indent(indent)
    push_indent()
    ret = mcgen('''
%(retval)sqmp_%(name)s(%(args)s&local_err);

''',
                name=c_name(name), args=arglist, retval=retval).rstrip()
                name=c_name(name), args=arglist, retval=retval)
    if ret_type:
        ret += "\n" + gen_err_check('local_err')
        ret += "\n" + mcgen(''''
%(marshal_output_call)s
''',
                            marshal_output_call=gen_marshal_output_call(name, ret_type)).rstrip()
    pop_indent(indent)
    return ret.rstrip()

        ret += gen_err_check('local_err')
        ret += mcgen('''

def gen_marshal_output_call(name, ret_type):
    if not ret_type:
        return ""
    return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_name(name)
qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
''',
                            c_name=c_name(name))
    pop_indent()
    return ret

def gen_visitor_input_containers_decl(args, obj):
def gen_visitor_input_containers_decl(args):
    ret = ""

    push_indent()
    if len(args) > 0:
        ret += mcgen('''
QmpInputVisitor *mi = qmp_input_visitor_new_strict(%(obj)s);
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
QapiDeallocVisitor *md;
Visitor *v;
''',
                     obj=obj)
''')
    pop_indent()

    return ret.rstrip()
    return ret

def gen_visitor_input_vars_decl(args):
    ret = ""
@@ -105,7 +99,7 @@ bool has_%(argname)s = false;
                         argname=c_name(argname), argtype=c_type(argtype))

    pop_indent()
    return ret.rstrip()
    return ret

def gen_visitor_input_block(args, dealloc=False):
    ret = ""
@@ -159,9 +153,9 @@ visit_type_%(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s);
qapi_dealloc_visitor_cleanup(md);
''')
    pop_indent()
    return ret.rstrip()
    return ret

def gen_marshal_output(name, args, ret_type, middle_mode):
def gen_marshal_output(name, ret_type):
    if not ret_type:
        return ""

@@ -194,14 +188,14 @@ out:

    return ret

def gen_marshal_input_decl(name, args, ret_type, middle_mode):
def gen_marshal_input_decl(name, middle_mode):
    ret = 'void qmp_marshal_input_%s(QDict *args, QObject **ret, Error **errp)' % c_name(name)
    if not middle_mode:
        ret = "static " + ret
    return ret

def gen_marshal_input(name, args, ret_type, middle_mode):
    hdr = gen_marshal_input_decl(name, args, ret_type, middle_mode)
    hdr = gen_marshal_input_decl(name, middle_mode)

    ret = mcgen('''
%(header)s
@@ -211,36 +205,24 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
                header=hdr)

    if ret_type:
        if is_c_ptr(ret_type):
            retval = "    %s retval = NULL;" % c_type(ret_type)
        else:
            retval = "    %s retval;" % c_type(ret_type)
        ret += mcgen('''
%(retval)s
    %(c_type)s retval;
''',
                     retval=retval)
                     c_type=c_type(ret_type))

    if len(args) > 0:
        ret += mcgen('''
%(visitor_input_containers_decl)s
%(visitor_input_vars_decl)s

%(visitor_input_block)s

''',
                     visitor_input_containers_decl=gen_visitor_input_containers_decl(args, "QOBJECT(args)"),
                     visitor_input_vars_decl=gen_visitor_input_vars_decl(args),
                     visitor_input_block=gen_visitor_input_block(args))
        ret += gen_visitor_input_containers_decl(args)
        ret += gen_visitor_input_vars_decl(args) + '\n'
        ret += gen_visitor_input_block(args) + '\n'
    else:
        ret += mcgen('''

    (void)args;

''')

    ret += mcgen('''
%(sync_call)s
''',
                 sync_call=gen_sync_call(name, args, ret_type, indent=4))
    ret += gen_sync_call(name, args, ret_type)

    if re.search('^ *goto out\\;', ret, re.MULTILINE):
        ret += mcgen('''

@@ -248,11 +230,11 @@ out:
''')
    ret += mcgen('''
    error_propagate(errp, local_err);
%(visitor_input_block_cleanup)s
''')
    ret += gen_visitor_input_block(args, dealloc=True)
    ret += mcgen('''
}
''',
                 visitor_input_block_cleanup=gen_visitor_input_block(args,
                                                                     dealloc=True))
''')
    return ret

def gen_registry(commands):
@@ -272,12 +254,13 @@ qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s, %(opts)s);
    ret = mcgen('''
static void qmp_init_marshal(void)
{
%(registry)s
''')
    ret += registry
    ret += mcgen('''
}

qapi_init(qmp_init_marshal);
''',
                registry=registry.rstrip())
''')
    return ret

middle_mode = False
@@ -357,14 +340,14 @@ for cmd in commands:
        arglist = cmd['data']
    if cmd.has_key('returns'):
        ret_type = cmd['returns']
    ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
    ret = generate_command_decl(cmd['command'], arglist, ret_type)
    fdecl.write(ret)
    if ret_type:
        ret = gen_marshal_output(cmd['command'], arglist, ret_type, middle_mode) + "\n"
        ret = gen_marshal_output(cmd['command'], ret_type) + "\n"
        fdef.write(ret)

    if middle_mode:
        fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], arglist, ret_type, middle_mode))
        fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], middle_mode))

    ret = gen_marshal_input(cmd['command'], arglist, ret_type, middle_mode) + "\n"
    fdef.write(ret)
+2 −4
Original line number Diff line number Diff line
@@ -167,8 +167,7 @@ extern const char *%(event_enum_name)s_lookup[];
                        event_enum_name = event_enum_name)

    enum_decl = mcgen('''
typedef enum %(event_enum_name)s
{
typedef enum %(event_enum_name)s {
''',
                      event_enum_name = event_enum_name)

@@ -199,7 +198,6 @@ const char *%(event_enum_name)s_lookup[] = {
''',
                event_enum_name = event_enum_name)

    i = 0
    for string in event_enum_strings:
        ret += mcgen('''
    "%(string)s",
@@ -267,7 +265,7 @@ fdecl.write(mcgen('''

exprs = parse_schema(input_file)

event_enum_name = prefix.upper().replace('-', '_') + "QAPIEvent"
event_enum_name = c_name(prefix + "QAPIEvent", protect=False)
event_enum_values = []
event_enum_strings = []

+60 −50
Original line number Diff line number Diff line
@@ -15,8 +15,7 @@ from qapi import *
def generate_fwd_builtin(name):
    return mcgen('''

typedef struct %(name)sList
{
typedef struct %(name)sList {
    union {
        %(type)s value;
        uint64_t padding;
@@ -32,8 +31,7 @@ def generate_fwd_struct(name):

typedef struct %(name)s %(name)s;

typedef struct %(name)sList
{
typedef struct %(name)sList {
    union {
        %(name)s *value;
        uint64_t padding;
@@ -45,8 +43,8 @@ typedef struct %(name)sList

def generate_fwd_enum_struct(name):
    return mcgen('''
typedef struct %(name)sList
{

typedef struct %(name)sList {
    union {
        %(name)s value;
        uint64_t padding;
@@ -79,8 +77,8 @@ def generate_struct(expr):
    base = expr.get('base')

    ret = mcgen('''
struct %(name)s
{

struct %(name)s {
''',
          name=c_name(structname))

@@ -105,10 +103,10 @@ struct %(name)s

def generate_enum_lookup(name, values):
    ret = mcgen('''

const char *const %(name)s_lookup[] = {
''',
                name=c_name(name))
    i = 0
    for value in values:
        index = c_enum_const(name, value)
        ret += mcgen('''
@@ -120,7 +118,6 @@ const char * const %(name)s_lookup[] = {
    ret += mcgen('''
    [%(max_index)s] = NULL,
};

''',
        max_index=max_index)
    return ret
@@ -128,13 +125,14 @@ const char * const %(name)s_lookup[] = {
def generate_enum(name, values):
    name = c_name(name)
    lookup_decl = mcgen('''

extern const char *const %(name)s_lookup[];
''',
                name=name)

    enum_decl = mcgen('''
typedef enum %(name)s
{

typedef enum %(name)s {
''',
                name=name)

@@ -156,7 +154,7 @@ typedef enum %(name)s
''',
                 name=name)

    return lookup_decl + enum_decl
    return enum_decl + lookup_decl

def generate_alternate_qtypes(expr):

@@ -164,6 +162,7 @@ def generate_alternate_qtypes(expr):
    members = expr['data']

    ret = mcgen('''

const int %(name)s_qtypes[QTYPE_MAX] = {
''',
                name=c_name(name))
@@ -199,15 +198,41 @@ def generate_union(expr, meta):
        discriminator_type_name = '%sKind' % (name)

    ret = mcgen('''
struct %(name)s
{

struct %(name)s {
''',
                name=name)
    if base:
        ret += mcgen('''
    /* Members inherited from %(c_name)s: */
''',
                     c_name=c_name(base))
        base_fields = find_struct(base)['data']
        ret += generate_struct_fields(base_fields)
        ret += mcgen('''
    /* Own members: */
''')
    else:
        assert not discriminator
        ret += mcgen('''
    %(discriminator_type_name)s kind;
    union {
        void *data;
''',
                name=name,
                     discriminator_type_name=c_name(discriminator_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(discriminator or 'kind'))

    for key in typeinfo:
        ret += mcgen('''
        %(c_type)s %(c_name)s;
@@ -217,17 +242,6 @@ struct %(name)s

    ret += mcgen('''
    };
''')

    if base:
        assert discriminator
        base_fields = find_struct(base)['data'].copy()
        del base_fields[discriminator]
        ret += generate_struct_fields(base_fields)
    else:
        assert not discriminator

    ret += mcgen('''
};
''')
    if meta == 'alternate':
@@ -314,14 +328,12 @@ fdef.write(mcgen('''
#include "qapi/dealloc-visitor.h"
#include "%(prefix)sqapi-types.h"
#include "%(prefix)sqapi-visit.h"

''',
                 prefix=prefix))

fdecl.write(mcgen('''
#include <stdbool.h>
#include <stdint.h>

'''))

exprs = parse_schema(input_file)
@@ -332,22 +344,22 @@ for typename in builtin_types.keys():
fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))

for expr in exprs:
    ret = "\n"
    ret = ""
    if expr.has_key('struct'):
        ret += generate_fwd_struct(expr['struct'])
    elif expr.has_key('enum'):
        ret += generate_enum(expr['enum'], expr['data']) + "\n"
        ret += generate_enum(expr['enum'], expr['data'])
        ret += generate_fwd_enum_struct(expr['enum'])
        fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
    elif expr.has_key('union'):
        ret += generate_fwd_struct(expr['union']) + "\n"
        ret += generate_fwd_struct(expr['union'])
        enum_define = discriminator_find_enum_define(expr)
        if not enum_define:
            ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
            fdef.write(generate_enum_lookup('%sKind' % expr['union'],
                                            expr['data'].keys()))
    elif expr.has_key('alternate'):
        ret += generate_fwd_struct(expr['alternate']) + "\n"
        ret += generate_fwd_struct(expr['alternate'])
        ret += generate_enum('%sKind' % expr['alternate'], expr['data'].keys())
        fdef.write(generate_enum_lookup('%sKind' % expr['alternate'],
                                        expr['data'].keys()))
@@ -367,34 +379,32 @@ fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
# have the functions defined, so we use -b option to provide control
# over these cases
if do_builtins:
    fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
    for typename in builtin_types.keys():
        fdef.write(generate_type_cleanup(typename + "List"))
    fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))

for expr in exprs:
    ret = "\n"
    ret = ""
    if expr.has_key('struct'):
        ret += generate_struct(expr) + "\n"
        ret += generate_type_cleanup_decl(expr['struct'] + "List")
        fdef.write(generate_type_cleanup(expr['struct'] + "List") + "\n")
        fdef.write(generate_type_cleanup(expr['struct'] + "List"))
        ret += generate_type_cleanup_decl(expr['struct'])
        fdef.write(generate_type_cleanup(expr['struct']) + "\n")
        fdef.write(generate_type_cleanup(expr['struct']))
    elif expr.has_key('union'):
        ret += generate_union(expr, 'union')
        ret += generate_union(expr, 'union') + "\n"
        ret += generate_type_cleanup_decl(expr['union'] + "List")
        fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
        fdef.write(generate_type_cleanup(expr['union'] + "List"))
        ret += generate_type_cleanup_decl(expr['union'])
        fdef.write(generate_type_cleanup(expr['union']) + "\n")
        fdef.write(generate_type_cleanup(expr['union']))
    elif expr.has_key('alternate'):
        ret += generate_union(expr, 'alternate')
        ret += generate_union(expr, 'alternate') + "\n"
        ret += generate_type_cleanup_decl(expr['alternate'] + "List")
        fdef.write(generate_type_cleanup(expr['alternate'] + "List") + "\n")
        fdef.write(generate_type_cleanup(expr['alternate'] + "List"))
        ret += generate_type_cleanup_decl(expr['alternate'])
        fdef.write(generate_type_cleanup(expr['alternate']) + "\n")
        fdef.write(generate_type_cleanup(expr['alternate']))
    elif expr.has_key('enum'):
        ret += generate_type_cleanup_decl(expr['enum'] + "List")
        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
        ret += "\n" + generate_type_cleanup_decl(expr['enum'] + "List")
        fdef.write(generate_type_cleanup(expr['enum'] + "List"))
    else:
        continue
    fdecl.write(ret)
+55 −38

File changed.

Preview size limit exceeded, changes collapsed.

Loading