Re: [PATCH v3 1/3] qapi: Add feature flags to commands in qapi

[Markus Armbruster]

Peter Krempa <address@hidden> writes:

> Similarly to features for struct types introduce the feature flags also
> for commands. This will allow notifying management layers of fixes and
> compatible changes in the behaviour of a command which may not be
> detectable any other way.
>
> The changes were heavily inspired by commit 6a8c0b51025.
>
> Signed-off-by: Peter Krempa <address@hidden>
> ---
>  docs/devel/qapi-code-gen.txt   |  7 ++++---
>  qapi/introspect.json           |  6 +++++-
>  scripts/qapi/commands.py       |  3 ++-
>  scripts/qapi/doc.py            |  3 ++-
>  scripts/qapi/expr.py           | 17 ++++++++++++++++-
>  scripts/qapi/introspect.py     |  7 ++++++-
>  scripts/qapi/schema.py         | 22 ++++++++++++++++++----
>  tests/qapi-schema/test-qapi.py |  3 ++-
>  8 files changed, 55 insertions(+), 13 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 64d9e4c6a9..637fa49977 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -640,9 +640,10 @@ change in the QMP syntax (usually by allowing values or 
> operations
>  that previously resulted in an error).  QMP clients may still need to
>  know whether the extension is available.
>
> -For this purpose, a list of features can be specified for a struct type.
> -This is exposed to the client as a list of string, where each string
> -signals that this build of QEMU shows a certain behaviour.
> +For this purpose, a list of features can be specified for a command or
> +struct type.  This is exposed to the client as a list of strings,
> +where each string signals that this build of QEMU shows a certain
> +behaviour.
>
>  Each member of the 'features' array defines a feature.  It can either
>  be { 'name': STRING, '*if': COND }, or STRING, which is shorthand for
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 1843c1cb17..031a954fa9 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -266,13 +266,17 @@
>  # @allow-oob: whether the command allows out-of-band execution,
>  #             defaults to false (Since: 2.12)
>  #
> +# @features: names of features associated with the command, in no particular
> +#            order. (since 4.2)
> +#
>  # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
>  #
>  # Since: 2.5
>  ##
>  { 'struct': 'SchemaInfoCommand',
>    'data': { 'arg-type': 'str', 'ret-type': 'str',
> -            '*allow-oob': 'bool' } }
> +            '*allow-oob': 'bool',
> +            '*features': [ 'str' ] } }
>
>  ##
>  # @SchemaInfoEvent:
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 898516b086..ab98e504f3 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -277,7 +277,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>          genc.add(gen_registry(self._regy.get_content(), self._prefix))
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          if not gen:
>              return
>          # FIXME: If T is a user-defined type, the user is responsible
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index dc8919bab7..8278ccbc43 100644
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -249,7 +249,8 @@ class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
>                                 body=texi_entity(doc, 'Members', ifcond)))
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          doc = self.cur_doc
>          if boxed:
>              body = texi_body(doc)
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index da23063f57..129a4075f0 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -277,12 +277,26 @@ def check_command(expr, info):
>      args = expr.get('data')
>      rets = expr.get('returns')
>      boxed = expr.get('boxed', False)
> +    features = expr.get('features')
>
>      if boxed and args is None:
>          raise QAPISemError(info, "'boxed': true requires 'data'")
>      check_type(args, info, "'data'", allow_dict=not boxed)
>      check_type(rets, info, "'returns'", allow_array=True)
>
> +    if features:
> +        if not isinstance(features, list):
> +            raise QAPISemError(info, "'features' must be an array")
> +        for f in features:
> +            source = "'features' member"
> +            assert isinstance(f, dict)
> +            check_keys(f, info, source, ['name'], ['if'])
> +            check_name_is_str(f['name'], info, source)
> +            source = "%s '%s'" % (source, f['name'])
> +            check_name_str(f['name'], info, source)
> +            check_if(f, info, source)
> +            normalize_if(f)
> +

Copied from check_struct().  Let's factor it into a helper function
check_features(features, info).

>
>  def check_event(expr, info):
>      args = expr.get('data')
> @@ -357,10 +371,11 @@ def check_exprs(exprs):
>          elif meta == 'command':
>              check_keys(expr, info, meta,
>                         ['command'],
> -                       ['data', 'returns', 'boxed', 'if',
> +                       ['data', 'returns', 'boxed', 'if', 'features',
>                          'gen', 'success-response', 'allow-oob',
>                          'allow-preconfig'])
>              normalize_members(expr.get('data'))
> +            normalize_features(expr.get('features'))
>              check_command(expr, info)
>          elif meta == 'event':
>              check_keys(expr, info, meta,
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index d1c1ad346d..739b35ae8f 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -209,13 +209,18 @@ const QLitObject %(c_name)s = %(c_string)s;
>                             for m in variants.variants]}, ifcond)
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          arg_type = arg_type or self._schema.the_empty_object_type
>          ret_type = ret_type or self._schema.the_empty_object_type
>          obj = {'arg-type': self._use_type(arg_type),
>                 'ret-type': self._use_type(ret_type)}
>          if allow_oob:
>              obj['allow-oob'] = allow_oob
> +
> +        if features:
> +            obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
> +

Copied from visit_object_type_flat().  Acceptable for now.

>          self._gen_qlit(name, 'command', obj, ifcond)
>
>      def visit_event(self, name, info, ifcond, arg_type, boxed):
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 38041098bd..8a48231766 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -109,7 +109,8 @@ class QAPISchemaVisitor(object):
>          pass
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          pass
>
>      def visit_event(self, name, info, ifcond, arg_type, boxed):
> @@ -658,10 +659,14 @@ class QAPISchemaCommand(QAPISchemaEntity):
>      meta = 'command'
>
>      def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
> -                 gen, success_response, boxed, allow_oob, allow_preconfig):
> +                 gen, success_response, boxed, allow_oob, allow_preconfig,
> +                 features):
>          QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
>          assert not arg_type or isinstance(arg_type, str)
>          assert not ret_type or isinstance(ret_type, str)
> +        for f in features:
> +            assert isinstance(f, QAPISchemaFeature)
> +            f.set_defined_in(name)

Copied from QAPISchemaObjectType.__init__().  Acceptable for now.

>          self._arg_type_name = arg_type
>          self.arg_type = None
>          self._ret_type_name = ret_type
> @@ -671,6 +676,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>          self.boxed = boxed
>          self.allow_oob = allow_oob
>          self.allow_preconfig = allow_preconfig
> +        self.features = features
>
>      def check(self, schema):
>          QAPISchemaEntity.check(self, schema)
> @@ -700,13 +706,19 @@ class QAPISchemaCommand(QAPISchemaEntity):
>                          "command's 'returns' cannot take %s"
>                          % self.ret_type.describe())
>
> +        # Features are in a name space separate from members
> +        seen = {}
> +        for f in self.features:
> +            f.check_clash(self.info, seen)
> +

Copied from QAPISchemaObjectType.check().  Acceptable for now.

>      def visit(self, visitor):
>          QAPISchemaEntity.visit(self, visitor)
>          visitor.visit_command(self.name, self.info, self.ifcond,
>                                self.arg_type, self.ret_type,
>                                self.gen, self.success_response,
>                                self.boxed, self.allow_oob,
> -                              self.allow_preconfig)
> +                              self.allow_preconfig,
> +                              self.features)
>
>
>  class QAPISchemaEvent(QAPISchemaEntity):
> @@ -983,6 +995,7 @@ class QAPISchema(object):
>          allow_oob = expr.get('allow-oob', False)
>          allow_preconfig = expr.get('allow-preconfig', False)
>          ifcond = expr.get('if')
> +        features = expr.get('features', [])
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
>                  name, info, doc, ifcond, 'arg', self._make_members(data, 
> info))
> @@ -991,7 +1004,8 @@ class QAPISchema(object):
>              rets = self._make_array_type(rets[0], info)
>          self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, 
> rets,
>                                             gen, success_response,
> -                                           boxed, allow_oob, 
> allow_preconfig))
> +                                           boxed, allow_oob, allow_preconfig,
> +                                           self._make_features(features, 
> info)))
>
>      def _def_event(self, expr, info, doc):
>          name = expr['event']
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 664254618a..e13c2e8671 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -72,7 +72,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          self._print_if(ifcond)
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          print('command %s %s -> %s'
>                % (name, arg_type and arg_type.name,
>                   ret_type and ret_type.name))

v2 had:

           print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
                 % (gen, success_response, boxed, allow_oob, allow_preconfig))
           self._print_if(ifcond)
  +        if features:
  +            for f in features:
  +                print('    feature %s' % f.name)
  +                self._print_if(f.ifcond, 8)

       def visit_event(self, name, info, ifcond, arg_type, boxed):
           print('event %s %s' % (name, arg_type and arg_type.name))

I see this is now in PATCH 2.  Okay.