Hi
On Wed, Sep 7, 2016 at 5:40 PM Markus Armbruster <arm...@redhat.com> wrote:
> Markus Armbruster <arm...@redhat.com> writes:
>
> > Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> >
> >> Hi
> >>
> >> On Tue, Sep 6, 2016 at 7:58 PM Markus Armbruster <arm...@redhat.com>
> wrote:
> >>
> >>> QAPI language design issues, copying Eric.
> >>>
> >>> Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> >>>
> >>> > Hi
> >>> >
> >>> > On Tue, Sep 6, 2016 at 5:00 PM Markus Armbruster <arm...@redhat.com>
> >>> wrote:
> [...]
> >>> >> Yet another option is to add 'ifdef' keys to the definitions
> >>> >> themselves, i.e.
> >>> >>
> >>> >> { 'command': 'query-gic-capabilities', 'returns':
> ['GICCapability'],
> >>> >> 'ifdef': 'TARGET_ARM' }
> >>> >>
> >>> >
> >>> > That's the worst of all options imho, as it makes it hard to filter
> out
> >>> > unions/enums, ex:
> >>> >
> >>> > @ -3446,8 +3466,10 @@
> >>> > 'testdev': 'ChardevCommon',
> >>> > 'stdio' : 'ChardevStdio',
> >>> > 'console': 'ChardevCommon',
> >>> > +#ifdef CONFIG_SPICE
> >>> > 'spicevmc' :
> >>> 'ChardevSpiceChannel',
> >>> > 'spiceport' :
> 'ChardevSpicePort',
> >>> > +#endif
> >>> > 'vc' : 'ChardevVC',
> >>> > 'ringbuf': 'ChardevRingbuf',
> >>>
> >>> Point taken.
> >>>
> >>> Fixable the same way as always when a definition needs to grow
> >>> additional properties, but the syntax only provides a single value:
> make
> >>> that single value an object, and the old non-object value syntactic
> >>> sugar for the equivalent object value. We've previously discussed this
> >>> technique in the context of giving command arguments default values.
> >>> I'm not saying this is what we should do here, only pointing out it's
> >>> possible.
> >>>
> >>
> >> Ok, but let's find something, if possible simple and convenient, no?
> >
> > I agree it needs to be simple, both the interface (QAPI language) and
> > the implementation. However, I don't like "first past the post". I
> > prefer to explore the design space a bit.
> >
> > So let me explore the "add 'ifdef' keys to definitions" corner of the
> > QAPI language design space.
> >
> > Easily done for top-level definitions, because they're all JSON objects.
> > Could even add it to the include directive if we wanted to.
> >
> > Less easily done for enumeration, struct, union and alternate members.
> > Note that command and event arguments specified inline are a special
> > case of struct members.
> >
> > The "can't specify additional stuff for struct members" problem isn't
> > new. We hacked around it to specify "optional": encode it into the
> > member name. Doesn't scale. We need to solve the problem to be able to
> > specify default values, and we already decided how: have an JSON object
> > instead of a mere JSON string, make the string syntax sugar for {
> > 'type': STRING }. See commit 6b5abc7 and the discussion in qemu-devel
> > leading up to it. For consistency, we'll do it for union and alternate
> > members, too.
> >
> > That leaves just enumeration members. The same technique applies.
> >
> > If I remember correctly, we only need conditional commands right now, to
> > avoid regressing query-commands. The more complicated member work could
> > be done later.
>
> To gauge whether this idea is practical, I implemented key 'if' for
> commands. It's just a sketch, and has a number of issues, which I
> marked FIXME.
>
Tbh, I think it is scratching the surface doing it only for commands.
Furthermore, it's pushing the burdden of conditional generation at a
different level, at the C level, which is probably not so nice if some day
it targets a different language.
If the main issue is using #\w* for preprocessing, we could use something
else like #! (oh no someone else did it, there are many ex where comments
are not just comments), or .\w* (.ifdef / .endif etc).
As you said, the main goal of this patch was to disable commands from the
qapi-dispatch that are currently disabled with the middle mode.
Do you think it would be reasonable to accept this simple approach for now,
until we find a better more suitable solution? After all, this is internal,
we can iterate. Or should we drop this patch for this series? I don't like
seeing it delayed by weeks...
>
> I ported qmp-commands.hx's #if to qapi-schema.json. The TARGET_FOO are
> poisoned, so I commented them out. There's a CONFIG_SPICE left, which
> will do for testing.
>
> I also turned key 'gen': false into 'if': false. Possibly a bad idea.
>
> Anyway, diffstat isn't bad:
>
> docs/qapi-code-gen.txt | 14 ++++++-----
> qapi-schema.json | 15 ++++++++---
> qapi/introspect.json | 2 +-
> scripts/qapi-commands.py | 12 +++++++--
> scripts/qapi-introspect.py | 22 ++++++++++------
> scripts/qapi.py | 40
> ++++++++++++++++++++++--------
> tests/qapi-schema/type-bypass-bad-gen.err | 2 +-
> tests/qapi-schema/type-bypass-bad-gen.json | 4 +--
> 8 files changed, 77 insertions(+), 34 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index de298dc..93e99d8 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -423,7 +423,7 @@ 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, 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.
> +one exception noted below when 'if': false is used.
>
> The 'returns' member describes what will appear in the "return" member
> of a Client JSON Protocol reply on successful completion of a command.
> @@ -431,8 +431,8 @@ The member is optional from the command declaration;
> if absent, the
> "return" member 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,
> -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
> +with one exception noted below when 'if':false 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
> @@ -475,16 +475,18 @@ arguments for the user's function out of an input
> QDict, calls the
> user's function, and if it succeeded, builds an output QObject from
> its return value.
>
> +FIXME document 'if'
> +
> In rare cases, QAPI cannot express a type-safe representation of a
> corresponding Client JSON Protocol command. You then have to suppress
> -generation of a marshalling function by including a key 'gen' with
> +generation of a marshalling function by including a key 'if' with
> boolean value false, and instead write your own function. Please try
> to avoid adding new commands that rely on this, and instead use
> type-safe unions. For an example of this usage:
>
> { 'command': 'netdev_add',
> - 'data': {'type': 'str', 'id': 'str'},
> - 'gen': false }
> + 'if': false,
> + 'data': {'type': 'str', 'id': 'str'} }
>
> Normally, the QAPI schema is used to describe synchronous exchanges,
> where a response is expected. But in some cases, the action of a
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c4f3674..ad0559e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1269,7 +1269,9 @@
> #
> # Since: 0.14.0
> ##
> -{ 'command': 'query-spice', 'returns': 'SpiceInfo' }
> +{ 'command': 'query-spice',
> + 'if': 'defined(CONFIG_SPICE)',
> + 'returns': 'SpiceInfo' }
>
> ##
> # @BalloonInfo:
> @@ -2355,6 +2357,7 @@
> # Since: 2.5
> ##
> { 'command': 'dump-skeys',
> +# 'if': 'defined(TARGET_S390X)',
> 'data': { 'filename': 'str' } }
>
> ##
> @@ -2380,8 +2383,8 @@
> # If @type is not a valid network backend, DeviceNotFound
> ##
> { 'command': 'netdev_add',
> - 'data': {'type': 'str', 'id': 'str'},
> - 'gen': false } # so we can get the additional arguments
> + 'if': false, # so we can get the additional arguments
> + 'data': {'type': 'str', 'id': 'str'} }
>
> ##
> # @netdev_del:
> @@ -4455,6 +4458,7 @@
> # Since: 2.1
> ##
> { 'command': 'rtc-reset-reinjection' }
> +# 'if': 'defined(TARGET_I386)'
>
> # Rocker ethernet network switch
> { 'include': 'qapi/rocker.json' }
> @@ -4525,7 +4529,10 @@
> #
> # Since: 2.6
> ##
> -{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
> +{ 'command': 'query-gic-capabilities',
> +# 'if': 'defined(TARGET_ARM)',
> + 'returns': ['GICCapability']
> +}
>
> ##
> # CpuInstanceProperties
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 3fd81fb..b8f421a 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -46,7 +46,7 @@
> ##
> { 'command': 'query-qmp-schema',
> 'returns': [ 'SchemaInfo' ],
> - 'gen': false } # just to simplify qmp_query_json()
> + 'if': false } # just to simplify qmp_query_json()
>
> ##
> # @SchemaMetaType
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index a06a2c4..f34e4cc 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -215,9 +215,13 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> self._visited_ret_types = None
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> - if not gen:
> + genif, success_response, boxed):
> + if genif is False:
> return
> + pp_if = gen_pp_if(genif)
> + pp_endif = gen_pp_endif(genif)
> + self.decl += pp_if
> + self.defn += pp_if # FIXME blank lines are off
> self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
> if ret_type and ret_type not in self._visited_ret_types:
> self._visited_ret_types.add(ret_type)
> @@ -226,7 +230,11 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> self.decl += gen_marshal_decl(name)
> self.defn += gen_marshal(name, arg_type, boxed, ret_type)
> if not middle_mode:
> + self._regy += pp_if
> self._regy += gen_register_command(name, success_response)
> + self._regy += pp_endif
> + self.decl += pp_endif
> + self.defn += pp_endif
>
>
> middle_mode = False
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 541644e..0d8cec7 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -30,8 +30,6 @@ def to_json(obj, level=0):
> ret = '{' + ', '.join(elts) + '}'
> else:
> assert False # not implemented
> - if level == 1:
> - ret = '\n' + ret
> return ret
>
>
> @@ -69,8 +67,15 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
> extern const char %(c_name)s[];
> ''',
> c_name=c_name(name))
> - lines = to_json(jsons).split('\n')
> - c_string = '\n '.join([to_c_string(line) for line in lines])
> + c_string = '"["'
> + for i in jsons:
> + js, genif = i
> + # FIXME blank lines are off
> + c_string += gen_pp_if(genif or True)
> + c_string += '\n ' + to_c_string(to_json(js) + ', ')
> + c_string += gen_pp_endif(genif or True)
> + # FIXME trailing comma (JSON sucks)
> + c_string += '\n "]"'
> self.defn = mcgen('''
> const char %(c_name)s[] = %(c_string)s;
> ''',
> @@ -111,12 +116,12 @@ const char %(c_name)s[] = %(c_string)s;
> return '[' + self._use_type(typ.element_type) + ']'
> return self._name(typ.name)
>
> - def _gen_json(self, name, mtype, obj):
> + def _gen_json(self, name, mtype, obj, genif=True):
> if mtype not in ('command', 'event', 'builtin', 'array'):
> name = self._name(name)
> obj['name'] = name
> obj['meta-type'] = mtype
> - self._jsons.append(obj)
> + self._jsons.append((obj, genif))
>
> def _gen_member(self, member):
> ret = {'name': member.name, 'type': self._use_type(member.type)}
> @@ -154,12 +159,13 @@ const char %(c_name)s[] = %(c_string)s;
> for m in variants.variants]})
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + genif, success_response, boxed):
> arg_type = arg_type or self._schema.the_empty_object_type
> ret_type = ret_type or self._schema.the_empty_object_type
> self._gen_json(name, 'command',
> {'arg-type': self._use_type(arg_type),
> - 'ret-type': self._use_type(ret_type)})
> + 'ret-type': self._use_type(ret_type)},
> + genif)
>
> def visit_event(self, name, info, arg_type, boxed):
> arg_type = arg_type or self._schema.the_empty_object_type
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 21bc32f..6c0cf9f 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -698,7 +698,13 @@ def check_keys(expr_elem, meta, required,
> optional=[]):
> raise QAPIExprError(info,
> "Unknown key '%s' in %s '%s'"
> % (key, meta, name))
> - if (key == 'gen' or key == 'success-response') and value is not
> False:
> + if (key == 'if'
> + and value is not False and not isinstance(value, str)):
> + # FIXME update error message
> + raise QAPIExprError(info,
> + "'%s' of %s '%s' should only use false
> value"
> + % (key, meta, name))
> + if (key == 'success-response') and value is not False:
> raise QAPIExprError(info,
> "'%s' of %s '%s' should only use false
> value"
> % (key, meta, name))
> @@ -737,7 +743,7 @@ def check_exprs(exprs):
> add_struct(expr, info)
> elif 'command' in expr:
> check_keys(expr_elem, 'command', [],
> - ['data', 'returns', 'gen', 'success-response',
> 'boxed'])
> + ['data', 'returns', 'if', 'success-response',
> 'boxed'])
> add_name(expr['command'], info, 'command')
> elif 'event' in expr:
> check_keys(expr_elem, 'event', [], ['data', 'boxed'])
> @@ -838,7 +844,7 @@ class QAPISchemaVisitor(object):
> pass
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + genif, success_response, boxed):
> pass
>
> def visit_event(self, name, info, arg_type, boxed):
> @@ -1180,8 +1186,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
>
>
> class QAPISchemaCommand(QAPISchemaEntity):
> - def __init__(self, name, info, arg_type, ret_type, gen,
> success_response,
> - boxed):
> + def __init__(self, name, info, arg_type, ret_type,
> + genif, success_response, boxed):
> QAPISchemaEntity.__init__(self, name, info)
> assert not arg_type or isinstance(arg_type, str)
> assert not ret_type or isinstance(ret_type, str)
> @@ -1189,7 +1195,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
> self.arg_type = None
> self._ret_type_name = ret_type
> self.ret_type = None
> - self.gen = gen
> + self.genif = genif
> self.success_response = success_response
> self.boxed = boxed
>
> @@ -1216,7 +1222,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
> def visit(self, visitor):
> visitor.visit_command(self.name, self.info,
> self.arg_type, self.ret_type,
> - self.gen, self.success_response, self.boxed)
> + self.genif, self.success_response,
> self.boxed)
>
>
> class QAPISchemaEvent(QAPISchemaEntity):
> @@ -1419,17 +1425,20 @@ class QAPISchema(object):
> name = expr['command']
> data = expr.get('data')
> rets = expr.get('returns')
> - gen = expr.get('gen', True)
> + genif = expr.get('if', True)
> success_response = expr.get('success-response', True)
> boxed = expr.get('boxed', False)
> if isinstance(data, OrderedDict):
> + # TODO apply genif to the implicit object type
> data = self._make_implicit_object_type(
> name, info, 'arg', self._make_members(data, info))
> if isinstance(rets, list):
> + # TODO apply genif to the implicit array type
> + # TODO disjunction of all the genif
> assert len(rets) == 1
> rets = self._make_array_type(rets[0], info)
> - self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
> - success_response, boxed))
> + self._def_entity(QAPISchemaCommand(name, info, data, rets,
> + genif, success_response,
> boxed))
>
> def _def_event(self, expr, info):
> name = expr['event']
> @@ -1704,6 +1713,17 @@ def gen_params(arg_type, boxed, extra):
> return ret
>
>
> +def gen_pp_if(cond):
> + if cond is True:
> + return ''
> + return '\n#if ' + cond + '\n'
> +
> +
> +def gen_pp_endif(cond):
> + if cond is True:
> + return ''
> + return '\n#endif /* ' + cond + ' */\n'
> +
> #
> # Common command line parsing
> #
> diff --git a/tests/qapi-schema/type-bypass-bad-gen.err
> b/tests/qapi-schema/type-bypass-bad-gen.err
> index a83c3c6..cca25f1 100644
> --- a/tests/qapi-schema/type-bypass-bad-gen.err
> +++ b/tests/qapi-schema/type-bypass-bad-gen.err
> @@ -1 +1 @@
> -tests/qapi-schema/type-bypass-bad-gen.json:2: 'gen' of command 'foo'
> should only use false value
> +tests/qapi-schema/type-bypass-bad-gen.json:2: 'if' of command 'foo'
> should only use false value
> diff --git a/tests/qapi-schema/type-bypass-bad-gen.json
> b/tests/qapi-schema/type-bypass-bad-gen.json
> index e8dec34..637b11f 100644
> --- a/tests/qapi-schema/type-bypass-bad-gen.json
> +++ b/tests/qapi-schema/type-bypass-bad-gen.json
> @@ -1,2 +1,2 @@
> -# 'gen' should only appear with value false
> -{ 'command': 'foo', 'gen': 'whatever' }
> +# 'if' should only appear with value false FIXME or str
> +{ 'command': 'foo', 'if': null }
>
--
Marc-André Lureau
