Commit 44bd1276 authored by Eric Blake's avatar Eric Blake Committed by Markus Armbruster
Browse files

qapi: Tighten checking of unions 



Previous commits demonstrated that the generator had several
flaws with less-than-perfect unions:
- a simple union that listed the same branch twice (or two variant
names that map to the same C enumerator, including the implicit
MAX sentinel) ended up generating invalid C code
- an anonymous union that listed two branches with the same qtype
ended up generating invalid C code
- the generator crashed on anonymous union attempts to use an
array type
- the generator was silently ignoring a base type for anonymous
unions
- the generator allowed unknown types or nested anonymous unions
as a branch in an anonymous union

Signed-off-by: default avatarEric Blake <eblake@redhat.com>
Reviewed-by: default avatarMarkus Armbruster <armbru@redhat.com>
Signed-off-by: default avatarMarkus Armbruster <armbru@redhat.com>
parent a8d4a2e4

scripts/qapi-types.py

+2 −11
Original line number Diff line number Diff line
@@ -181,17 +181,8 @@ const int %(name)s_qtypes[QTYPE_MAX] = { 
    name=name)



    for key in members:

        qapi_type = members[key]

        if builtin_types.has_key(qapi_type):

            qtype = builtin_types[qapi_type]

        elif find_struct(qapi_type):

            qtype = "QTYPE_QDICT"

        elif find_union(qapi_type):

            qtype = "QTYPE_QDICT"

        elif find_enum(qapi_type):

            qtype = "QTYPE_QSTRING"

        else:

            assert False, "Invalid anonymous union member"

        qtype = find_anonymous_member_qtype(members[key])

        assert qtype, "Invalid anonymous union member"



        ret += mcgen('''

    [ %(qtype)s ] = %(abbrev)s_KIND_%(enum)s,

scripts/qapi.py

+72 −17
Original line number Diff line number Diff line
@@ -224,6 +224,23 @@ def find_base_fields(base): 
        return None

    return base_struct_define['data']



# Return the qtype of an anonymous union branch, or None on error.

def find_anonymous_member_qtype(qapi_type):

    if builtin_types.has_key(qapi_type):

        return builtin_types[qapi_type]

    elif find_struct(qapi_type):

        return "QTYPE_QDICT"

    elif find_enum(qapi_type):

        return "QTYPE_QSTRING"

    else:

        union = find_union(qapi_type)

        if union:

            discriminator = union.get('discriminator')

            if discriminator == {}:

                return None

            return "QTYPE_QDICT"

    return None



# Return the discriminator enum define if discriminator is specified as an

# enum type, otherwise return None.

def discriminator_find_enum_define(expr):
@@ -258,6 +275,8 @@ def check_union(expr, expr_info): 
    base = expr.get('base')

    discriminator = expr.get('discriminator')

    members = expr['data']

    values = { 'MAX': '(automatic)' }

    types_seen = {}



    # If the object has a member 'base', its value must name a complex type,

    # and there must be a discriminator.
@@ -266,26 +285,35 @@ def check_union(expr, expr_info): 
            raise QAPIExprError(expr_info,

                                "Union '%s' requires a discriminator to go "

                                "along with base" %name)

        base_fields = find_base_fields(base)

        if not base_fields:

            raise QAPIExprError(expr_info,

                                "Base '%s' is not a valid type"

                                % base)



    # If the union object has no member 'discriminator', it's a

    # simple union. If 'discriminator' is {}, it is an anonymous union.

    if not discriminator or discriminator == {}:

    if discriminator is None or discriminator == {}:

        enum_define = None

        if base is not None:

            raise QAPIExprError(expr_info,

                                "Union '%s' must not have a base"

                                % name)



    # Else, it's a flat union.

    else:

        # The object must have a member 'base'.

        if not base:

        # The object must have a string member 'base'.

        if not isinstance(base, str):

            raise QAPIExprError(expr_info,

                                "Flat union '%s' must have a base field"

                                "Flat union '%s' must have a string base field"

                                % name)

        base_fields = find_base_fields(base)

        if not base_fields:

            raise QAPIExprError(expr_info,

                                "Base '%s' is not a valid type"

                                % base)



        # The value of member 'discriminator' must name a member of the

        # base type.

        if not isinstance(discriminator, str):

            raise QAPIExprError(expr_info,

                                "Flat union '%s' discriminator must be a string"

                                % name)

        discriminator_type = base_fields.get(discriminator)

        if not discriminator_type:

            raise QAPIExprError(expr_info,
@@ -301,15 +329,42 @@ def check_union(expr, expr_info): 


    # Check every branch

    for (key, value) in members.items():

        # If this named member's value names an enum type, then all members

        # If the discriminator names an enum type, then all members

        # of 'data' must also be members of the enum type.

        if enum_define and not key in enum_define['enum_values']:

        if enum_define:

            if not key in enum_define['enum_values']:

                raise QAPIExprError(expr_info,

                                    "Discriminator value '%s' is not found in "

                                    "enum '%s'" %

                                    (key, enum_define["enum_name"]))

        # Todo: add checking for values. Key is checked as above, value can be

        # also checked here, but we need more functions to handle array case.



        # Otherwise, check for conflicts in the generated enum

        else:

            c_key = _generate_enum_string(key)

            if c_key in values:

                raise QAPIExprError(expr_info,

                                    "Union '%s' member '%s' clashes with '%s'"

                                    % (name, key, values[c_key]))

            values[c_key] = key



        # Ensure anonymous unions have no type conflicts.

        if discriminator == {}:

            if isinstance(value, list):

                raise QAPIExprError(expr_info,

                                    "Anonymous union '%s' member '%s' must "

                                    "not be array type" % (name, key))

            qtype = find_anonymous_member_qtype(value)

            if not qtype:

                raise QAPIExprError(expr_info,

                                    "Anonymous union '%s' member '%s' has "

                                    "invalid type '%s'" % (name, key, value))

            if qtype in types_seen:

                raise QAPIExprError(expr_info,

                                    "Anonymous union '%s' member '%s' can't "

                                    "be distinguished from member '%s'"

                                    % (name, key, types_seen[qtype]))

            types_seen[qtype] = key





def check_enum(expr, expr_info):

    name = expr['enum']

tests/qapi-schema/alternate-array.err

+1 −0
Original line number Diff line number Diff line 
tests/qapi-schema/alternate-array.json:5: Anonymous union 'MyUnion' member 'two' must not be array type

tests/qapi-schema/alternate-array.exit

+1 −1
Original line number Diff line number Diff line 
0
 
1

tests/qapi-schema/alternate-array.json

+1 −1
Original line number Diff line number Diff line 
# FIXME: we should not allow array branches in anonymous unions

# we do not allow array branches in anonymous unions

# TODO: should we support this?

{ 'type': 'One',

  'data': { 'name': 'str' } }