Kashyap Chamarthy <kchamart@redhat.com> writes:
On Mon, Oct 25, 2021 at 07:25:24AM +0200, Markus Armbruster wrote:
By convention, names starting with "x-" are experimental. The parts of external interfaces so named may be withdrawn or changed incompatibly in future releases.
Drawback: promoting something from experimental to stable involves a name change. Client code needs to be updated.
Moreover, the convention is not universally observed:
* QOM type "input-barrier" has properties "x-origin", "y-origin". Looks accidental, but it's ABI since 4.2.
* QOM types "memory-backend-file", "memory-backend-memfd", "memory-backend-ram", and "memory-backend-epc" have a property "x-use-canonical-path-for-ramblock-id" that is documented to be stable despite its name.
Looks like there's another stable property with an "x-" prefix: "x-remote-object", part of QOM type @RemoteObjectProperties.
The union branch 'x-remote-object' isn't flagged 'unstable' (because union branches can't have feature flags), but the enumeration value 'x-remote-object' is. Sufficient, because you can't use the branch without using the enumeration value. Admittedly subtle. I wrote a bit of code (appended) to make sure I don't miss names.
Given the above "x-" properties are now stable, I take it that they cannot be renamed now, as they might break any tools using them? My guess is the tedious way is not worth it: deprecate them, and add the non-x variants as "synonyms".
"x-use-canonical-path-for-ramblock-id" goes back to commit fa0cb34d22 "hostmem: use object id for memory region name with >= 4.0" (v4.0). It may have been intended to be internal back then. It wasn't anymore when commit 8db0b20415 "machine: add missing doc for memory-backend option" (v6.0) documented it: And document that x-use-canonical-path-for-ramblock-id, is considered to be stable to make sure it won't go away by accident. x- was intended for unstable/iternal properties, and not supposed to be stable option. However it's too late to rename (drop x-) it as it would mean that users will have to mantain both x-use-canonical-path-for-ramblock-id (for QEMU 5.0-5.2) versions and prefix-less for later versions. Igor's reasoning still applies. "x-origin" has always been stable. Same argument.
We could document these exceptions, but documentation helps only humans. We want to recognize "unstable" in code, like "deprecated".
Replace the convention by a new special feature flag "unstable". It will be recognized by the QAPI generator, like the existing feature flag "deprecated", and unlike regular feature flags.
FWIW, sounds good to me.
Thanks!
This commit updates documentation and prepares tests. The next commit updates the QAPI schema. The remaining patches update the QAPI generator and wire up -compat policy checking.
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/devel/qapi-code-gen.rst | 9 ++++++--- tests/qapi-schema/qapi-schema-test.json | 7 +++++-- tests/qapi-schema/qapi-schema-test.out | 5 +++++ 3 files changed, 16 insertions(+), 5 deletions(-)
[...]
commit 415b71a9f6e5bc37e84895d2e767cf4cfacd279b (HEAD) Author: Markus Armbruster <armbru@redhat.com> Date: Sat Oct 9 09:01:21 2021 +0200 qapi: Find x- without feature unstable DBG diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index b7b3fc0ce4..f2af1d7eea 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -14,6 +14,8 @@ # TODO catching name collisions in generated code would be nice +import sys + from collections import OrderedDict import os import re @@ -118,6 +120,11 @@ def describe(self): return "%s '%s'" % (self.meta, self.name) +def check_have_feature_unstable(name, info, features): + if name.startswith('x-') and 'unstable' not in (f.name for f in features): + print(QAPISemError(info, f"XXX %{name} %{features}"), file=sys.stderr) + + class QAPISchemaVisitor: def visit_begin(self, schema): pass @@ -718,6 +725,7 @@ def __init__(self, name, info, ifcond=None, features=None): self.features = features or [] def connect_doc(self, doc): + check_have_feature_unstable(self.name, self.info, self.features) super().connect_doc(doc) if doc: for f in self.features: @@ -745,6 +753,7 @@ def __init__(self, name, info, typ, optional, ifcond=None, features=None): self.features = features or [] def check(self, schema): + check_have_feature_unstable(self.name, self.info, self.features) assert self.defined_in self.type = schema.resolve_type(self._type_name, self.info, self.describe) @@ -789,6 +798,7 @@ def __init__(self, name, info, doc, ifcond, features, def check(self, schema): super().check(schema) + check_have_feature_unstable(self.name, self.info, self.features) if self._arg_type_name: self.arg_type = schema.resolve_type( self._arg_type_name, self.info, "command's 'data'") @@ -844,6 +854,7 @@ def __init__(self, name, info, doc, ifcond, features, arg_type, boxed): def check(self, schema): super().check(schema) + check_have_feature_unstable(self.name, self.info, self.features) if self._arg_type_name: self.arg_type = schema.resolve_type( self._arg_type_name, self.info, "event's 'data'")