scripts/qapi/backend.py | 63 ++++++++++++++++++++++++++++++++++ scripts/qapi/main.py | 75 ++++++++++++++++++++++------------------- 2 files changed, 103 insertions(+), 35 deletions(-) create mode 100644 scripts/qapi/backend.py
The 'qapi.backend.QAPIBackend' class defines an API contract for code
generators. The current generator is put into a new class
'qapi.backend.QAPICBackend' and made to be the default impl.
A custom generator can be requested using the '-k' arg which takes a
fully qualified python class name
qapi-gen.py -B the.python.module.QAPIMyBackend
This allows out of tree code to use the QAPI generator infrastructure
to create new language bindings for QAPI schemas. This has the caveat
that the QAPI generator APIs are not guaranteed stable, so consumers
of this feature may have to update their code to be compatible with
future QEMU releases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
In v2:
- Create QAPISchema object ahead of calling backend
- Use -B instead of -k
- Fix mypy annotations
- Add error checking when loading backend
- Hardcode import of QAPICBackend to guarantee mypy coverage
scripts/qapi/backend.py | 63 ++++++++++++++++++++++++++++++++++
scripts/qapi/main.py | 75 ++++++++++++++++++++++-------------------
2 files changed, 103 insertions(+), 35 deletions(-)
create mode 100644 scripts/qapi/backend.py
diff --git a/scripts/qapi/backend.py b/scripts/qapi/backend.py
new file mode 100644
index 0000000000..14e60aa67a
--- /dev/null
+++ b/scripts/qapi/backend.py
@@ -0,0 +1,63 @@
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+from abc import ABC, abstractmethod
+
+from .commands import gen_commands
+from .events import gen_events
+from .features import gen_features
+from .introspect import gen_introspect
+from .schema import QAPISchema
+from .types import gen_types
+from .visit import gen_visit
+
+
+class QAPIBackend(ABC):
+
+ @abstractmethod
+ def generate(self,
+ schema: QAPISchema,
+ output_dir: str,
+ prefix: str,
+ unmask: bool,
+ builtins: bool,
+ gen_tracing: bool) -> None:
+ """
+ Generate code for the given schema into the target directory.
+
+ :param schema: The primary QAPI schema object.
+ :param output_dir: The output directory to store generated code.
+ :param prefix: Optional C-code prefix for symbol names.
+ :param unmask: Expose non-ABI names through introspection?
+ :param builtins: Generate code for built-in types?
+
+ :raise QAPIError: On failures.
+ """
+
+
+class QAPICBackend(QAPIBackend):
+
+ def generate(self,
+ schema: QAPISchema,
+ output_dir: str,
+ prefix: str,
+ unmask: bool,
+ builtins: bool,
+ gen_tracing: bool) -> None:
+ """
+ Generate C code for the given schema into the target directory.
+
+ :param schema_file: The primary QAPI schema file.
+ :param output_dir: The output directory to store generated code.
+ :param prefix: Optional C-code prefix for symbol names.
+ :param unmask: Expose non-ABI names through introspection?
+ :param builtins: Generate code for built-in types?
+
+ :raise QAPIError: On failures.
+ """
+ gen_types(schema, output_dir, prefix, builtins)
+ gen_features(schema, output_dir, prefix)
+ gen_visit(schema, output_dir, prefix, builtins)
+ gen_commands(schema, output_dir, prefix, gen_tracing)
+ gen_events(schema, output_dir, prefix)
+ gen_introspect(schema, output_dir, prefix, unmask)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 324081b9fc..8a8b1e0121 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -8,18 +8,14 @@
"""
import argparse
+from importlib import import_module
import sys
from typing import Optional
-from .commands import gen_commands
+from .backend import QAPIBackend, QAPICBackend
from .common import must_match
from .error import QAPIError
-from .events import gen_events
-from .features import gen_features
-from .introspect import gen_introspect
from .schema import QAPISchema
-from .types import gen_types
-from .visit import gen_visit
def invalid_prefix_char(prefix: str) -> Optional[str]:
@@ -29,32 +25,37 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
return None
-def generate(schema_file: str,
- output_dir: str,
- prefix: str,
- unmask: bool = False,
- builtins: bool = False,
- gen_tracing: bool = False) -> None:
- """
- Generate C code for the given schema into the target directory.
+def create_backend(path: str) -> QAPIBackend:
+ if path is None:
+ return QAPICBackend()
- :param schema_file: The primary QAPI schema file.
- :param output_dir: The output directory to store generated code.
- :param prefix: Optional C-code prefix for symbol names.
- :param unmask: Expose non-ABI names through introspection?
- :param builtins: Generate code for built-in types?
+ if "." not in path:
+ print(f"Missing qualified module path in '{path}'", file=sys.stderr)
+ sys.exit(1)
- :raise QAPIError: On failures.
- """
- assert invalid_prefix_char(prefix) is None
+ module_path, _, class_name = path.rpartition('.')
+ try:
+ mod = import_module(module_path)
+ except Exception as ex:
+ print(f"Unable to import '{module_path}': {ex}", file=sys.stderr)
+ sys.exit(1)
+
+ if not hasattr(mod, class_name):
+ print(f"Module '{module_path}' has no class '{class_name}'", file=sys.stderr)
+ sys.exit(1)
+ klass = getattr(mod, class_name)
+
+ try:
+ backend = klass()
+
+ if not isinstance(backend, QAPIBackend):
+ print(f"Backend '{path}' must be an instance of QAPIBackend", file=sys.stderr)
+ sys.exit(1)
- schema = QAPISchema(schema_file)
- gen_types(schema, output_dir, prefix, builtins)
- gen_features(schema, output_dir, prefix)
- gen_visit(schema, output_dir, prefix, builtins)
- gen_commands(schema, output_dir, prefix, gen_tracing)
- gen_events(schema, output_dir, prefix)
- gen_introspect(schema, output_dir, prefix, unmask)
+ return backend
+ except Exception as ex:
+ print(f"Backend '{path}' cannot be instantiated: {ex}", file=sys.stderr)
+ sys.exit(1)
def main() -> int:
@@ -77,6 +78,8 @@ def main() -> int:
parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
dest='unmask',
help="expose non-ABI names in introspection")
+ parser.add_argument('-B', '--backend', default=None,
+ help="Python module name for code generator")
# Option --suppress-tracing exists so we can avoid solving build system
# problems. TODO Drop it when we no longer need it.
@@ -93,12 +96,14 @@ def main() -> int:
return 1
try:
- generate(args.schema,
- output_dir=args.output_dir,
- prefix=args.prefix,
- unmask=args.unmask,
- builtins=args.builtins,
- gen_tracing=not args.suppress_tracing)
+ schema = QAPISchema(args.schema)
+ backend = create_backend(args.backend)
+ backend.generate(schema,
+ output_dir=args.output_dir,
+ prefix=args.prefix,
+ unmask=args.unmask,
+ builtins=args.builtins,
+ gen_tracing=not args.suppress_tracing)
except QAPIError as err:
print(err, file=sys.stderr)
return 1
--
2.47.1
Daniel P. Berrangé <berrange@redhat.com> writes:
> The 'qapi.backend.QAPIBackend' class defines an API contract for code
> generators. The current generator is put into a new class
> 'qapi.backend.QAPICBackend' and made to be the default impl.
>
> A custom generator can be requested using the '-k' arg which takes a
Missed an instance of -k. Can fix this myself.
> fully qualified python class name
>
> qapi-gen.py -B the.python.module.QAPIMyBackend
>
> This allows out of tree code to use the QAPI generator infrastructure
> to create new language bindings for QAPI schemas. This has the caveat
> that the QAPI generator APIs are not guaranteed stable, so consumers
> of this feature may have to update their code to be compatible with
> future QEMU releases.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>
> In v2:
>
> - Create QAPISchema object ahead of calling backend
> - Use -B instead of -k
> - Fix mypy annotations
> - Add error checking when loading backend
> - Hardcode import of QAPICBackend to guarantee mypy coverage
>
> scripts/qapi/backend.py | 63 ++++++++++++++++++++++++++++++++++
> scripts/qapi/main.py | 75 ++++++++++++++++++++++-------------------
> 2 files changed, 103 insertions(+), 35 deletions(-)
> create mode 100644 scripts/qapi/backend.py
>
> diff --git a/scripts/qapi/backend.py b/scripts/qapi/backend.py
> new file mode 100644
> index 0000000000..14e60aa67a
> --- /dev/null
> +++ b/scripts/qapi/backend.py
> @@ -0,0 +1,63 @@
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +from abc import ABC, abstractmethod
> +
> +from .commands import gen_commands
> +from .events import gen_events
> +from .features import gen_features
> +from .introspect import gen_introspect
> +from .schema import QAPISchema
> +from .types import gen_types
> +from .visit import gen_visit
> +
> +
> +class QAPIBackend(ABC):
> +
> + @abstractmethod
> + def generate(self,
> + schema: QAPISchema,
> + output_dir: str,
> + prefix: str,
> + unmask: bool,
> + builtins: bool,
> + gen_tracing: bool) -> None:
> + """
> + Generate code for the given schema into the target directory.
> +
> + :param schema: The primary QAPI schema object.
> + :param output_dir: The output directory to store generated code.
> + :param prefix: Optional C-code prefix for symbol names.
> + :param unmask: Expose non-ABI names through introspection?
> + :param builtins: Generate code for built-in types?
> +
> + :raise QAPIError: On failures.
> + """
> +
> +
> +class QAPICBackend(QAPIBackend):
> +
> + def generate(self,
> + schema: QAPISchema,
> + output_dir: str,
> + prefix: str,
> + unmask: bool,
> + builtins: bool,
> + gen_tracing: bool) -> None:
> + """
> + Generate C code for the given schema into the target directory.
> +
> + :param schema_file: The primary QAPI schema file.
> + :param output_dir: The output directory to store generated code.
> + :param prefix: Optional C-code prefix for symbol names.
> + :param unmask: Expose non-ABI names through introspection?
> + :param builtins: Generate code for built-in types?
> +
> + :raise QAPIError: On failures.
> + """
> + gen_types(schema, output_dir, prefix, builtins)
> + gen_features(schema, output_dir, prefix)
> + gen_visit(schema, output_dir, prefix, builtins)
> + gen_commands(schema, output_dir, prefix, gen_tracing)
> + gen_events(schema, output_dir, prefix)
> + gen_introspect(schema, output_dir, prefix, unmask)
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 324081b9fc..8a8b1e0121 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -8,18 +8,14 @@
> """
>
> import argparse
> +from importlib import import_module
> import sys
> from typing import Optional
>
> -from .commands import gen_commands
> +from .backend import QAPIBackend, QAPICBackend
> from .common import must_match
> from .error import QAPIError
> -from .events import gen_events
> -from .features import gen_features
> -from .introspect import gen_introspect
> from .schema import QAPISchema
> -from .types import gen_types
> -from .visit import gen_visit
>
>
> def invalid_prefix_char(prefix: str) -> Optional[str]:
> @@ -29,32 +25,37 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
> return None
>
>
> -def generate(schema_file: str,
> - output_dir: str,
> - prefix: str,
> - unmask: bool = False,
> - builtins: bool = False,
> - gen_tracing: bool = False) -> None:
> - """
> - Generate C code for the given schema into the target directory.
> +def create_backend(path: str) -> QAPIBackend:
> + if path is None:
> + return QAPICBackend()
>
> - :param schema_file: The primary QAPI schema file.
> - :param output_dir: The output directory to store generated code.
> - :param prefix: Optional C-code prefix for symbol names.
> - :param unmask: Expose non-ABI names through introspection?
> - :param builtins: Generate code for built-in types?
> + if "." not in path:
> + print(f"Missing qualified module path in '{path}'", file=sys.stderr)
> + sys.exit(1)
>
> - :raise QAPIError: On failures.
> - """
> - assert invalid_prefix_char(prefix) is None
> + module_path, _, class_name = path.rpartition('.')
I'd like to tweak this to
module_path, dot, class_name = path.rpartition('.')
if not dot:
print(f"argument of -B must be of the form MODULE.CLASS",
file=sys.stderr)
sys.exit(1)
> + try:
> + mod = import_module(module_path)
> + except Exception as ex:
pylint complains:
scripts/qapi/main.py:39:11: W0718: Catching too general exception Exception (broad-exception-caught)
I can't see offhand what exception(s) we're supposed to catch here, so
let's not worry about this now.
> + print(f"Unable to import '{module_path}': {ex}", file=sys.stderr)
> + sys.exit(1)
> +
> + if not hasattr(mod, class_name):
> + print(f"Module '{module_path}' has no class '{class_name}'", file=sys.stderr)
pycodestyle-3 complains:
scripts/qapi/main.py:44:80: E501 line too long (85 > 79 characters)
Let's break the line after the comma, and also start the error message
in lower case for consistency with error messages elsewhere.
> + sys.exit(1)
> + klass = getattr(mod, class_name)
Alternatively
try:
klass = getattr(mod, class_name)
except AttributeError:
print(f"module '{module_path}' has no class '{class_name}'",
file=sys.stderr)
sys.exit(1)
Admittedly a matter of taste. I tend to avoid the
if frobnicate would fail:
error out
frobnicate
pattern when practical.
> +
> + try:
> + backend = klass()
> +
> + if not isinstance(backend, QAPIBackend):
> + print(f"Backend '{path}' must be an instance of QAPIBackend", file=sys.stderr)
Likewise.
> + sys.exit(1)
>
> - schema = QAPISchema(schema_file)
> - gen_types(schema, output_dir, prefix, builtins)
> - gen_features(schema, output_dir, prefix)
> - gen_visit(schema, output_dir, prefix, builtins)
> - gen_commands(schema, output_dir, prefix, gen_tracing)
> - gen_events(schema, output_dir, prefix)
> - gen_introspect(schema, output_dir, prefix, unmask)
> + return backend
> + except Exception as ex:
Likewise too general exception.
I'd like to shrink the try block and reduce the nesting:
try:
backend = klass()
except Exception as ex:
print(f"Backend '{path}' cannot be instantiated: {ex}", file=sys.stderr)
sys.exit(1)
if not isinstance(backend, QAPIBackend):
print(f"Backend '{path}' must be an instance of QAPIBackend", file=sys.stderr)
sys.exit(1)
return backend
> + print(f"Backend '{path}' cannot be instantiated: {ex}", file=sys.stderr)
Likewise line too long.
> + sys.exit(1)
>
>
> def main() -> int:
> @@ -77,6 +78,8 @@ def main() -> int:
> parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
> dest='unmask',
> help="expose non-ABI names in introspection")
> + parser.add_argument('-B', '--backend', default=None,
> + help="Python module name for code generator")
>
> # Option --suppress-tracing exists so we can avoid solving build system
> # problems. TODO Drop it when we no longer need it.
> @@ -93,12 +96,14 @@ def main() -> int:
> return 1
>
> try:
> - generate(args.schema,
> - output_dir=args.output_dir,
> - prefix=args.prefix,
> - unmask=args.unmask,
> - builtins=args.builtins,
> - gen_tracing=not args.suppress_tracing)
> + schema = QAPISchema(args.schema)
> + backend = create_backend(args.backend)
> + backend.generate(schema,
> + output_dir=args.output_dir,
> + prefix=args.prefix,
> + unmask=args.unmask,
> + builtins=args.builtins,
> + gen_tracing=not args.suppress_tracing)
> except QAPIError as err:
> print(err, file=sys.stderr)
> return 1
Error reporting test cases:
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent qapi/qapi-schema.json
argument of -B must be of the form MODULE.CLASS
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.backend.foo qapi/qapi-schema.json
module 'qapi.backend' has no class 'foo'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent qapi/qapi-schema.json
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent qapi/qapi-schema.json
argument of -B must be of the form MODULE.CLASS
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B nonexistent.foo qapi/qapi-schema.json
unable to import 'nonexistent': No module named 'nonexistent'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.backend.foo qapi/qapi-schema.json
module 'qapi.backend' has no class 'foo'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.backend.QAPIBackend qapi/qapi-schema.json
backend 'qapi.backend.QAPIBackend' cannot be instantiated: Can't instantiate abstract class QAPIBackend without an implementation for abstract method 'generate'
$ python3 scripts/qapi-gen.py -o /tmp/$$ -b -B qapi.common.Indentation qapi/qapi-schema.json
backend 'qapi.common.Indentation' must be an instance of QAPIBackend
Good enough.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
No need to respin, I can make the tweaks I suggested myself. Feel free
to challenge my suggestions, of course.
Markus Armbruster <armbru@redhat.com> writes:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> The 'qapi.backend.QAPIBackend' class defines an API contract for code
>> generators. The current generator is put into a new class
>> 'qapi.backend.QAPICBackend' and made to be the default impl.
>>
>> A custom generator can be requested using the '-k' arg which takes a
>
> Missed an instance of -k. Can fix this myself.
>
>> fully qualified python class name
>>
>> qapi-gen.py -B the.python.module.QAPIMyBackend
>>
>> This allows out of tree code to use the QAPI generator infrastructure
>> to create new language bindings for QAPI schemas. This has the caveat
>> that the QAPI generator APIs are not guaranteed stable, so consumers
>> of this feature may have to update their code to be compatible with
>> future QEMU releases.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[...]
>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
>> index 324081b9fc..8a8b1e0121 100644
>> --- a/scripts/qapi/main.py
>> +++ b/scripts/qapi/main.py
[...]
>> @@ -29,32 +25,37 @@ def invalid_prefix_char(prefix: str) -> Optional[str]:
>> return None
>>
>>
>> -def generate(schema_file: str,
>> - output_dir: str,
>> - prefix: str,
>> - unmask: bool = False,
>> - builtins: bool = False,
>> - gen_tracing: bool = False) -> None:
>> - """
>> - Generate C code for the given schema into the target directory.
>> +def create_backend(path: str) -> QAPIBackend:
>> + if path is None:
>> + return QAPICBackend()
>>
>> - :param schema_file: The primary QAPI schema file.
>> - :param output_dir: The output directory to store generated code.
>> - :param prefix: Optional C-code prefix for symbol names.
>> - :param unmask: Expose non-ABI names through introspection?
>> - :param builtins: Generate code for built-in types?
>> + if "." not in path:
>> + print(f"Missing qualified module path in '{path}'", file=sys.stderr)
>> + sys.exit(1)
>>
>> - :raise QAPIError: On failures.
>> - """
>> - assert invalid_prefix_char(prefix) is None
>> + module_path, _, class_name = path.rpartition('.')
>
> I'd like to tweak this to
>
> module_path, dot, class_name = path.rpartition('.')
> if not dot:
> print(f"argument of -B must be of the form MODULE.CLASS",
> file=sys.stderr)
This bothers flake8:
scripts/qapi/main.py:34:15: F541 f-string is missing placeholders
I'll make it a plain string instead.
> sys.exit(1)
>
[...]
On Tue, Feb 25, 2025 at 01:31:56PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > The 'qapi.backend.QAPIBackend' class defines an API contract for code
> > generators. The current generator is put into a new class
> > 'qapi.backend.QAPICBackend' and made to be the default impl.
> >
> > A custom generator can be requested using the '-k' arg which takes a
>
> Missed an instance of -k. Can fix this myself.
>
> > fully qualified python class name
> >
> > qapi-gen.py -B the.python.module.QAPIMyBackend
> >
> > This allows out of tree code to use the QAPI generator infrastructure
> > to create new language bindings for QAPI schemas. This has the caveat
> > that the QAPI generator APIs are not guaranteed stable, so consumers
> > of this feature may have to update their code to be compatible with
> > future QEMU releases.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > -def generate(schema_file: str,
> > - output_dir: str,
> > - prefix: str,
> > - unmask: bool = False,
> > - builtins: bool = False,
> > - gen_tracing: bool = False) -> None:
> > - """
> > - Generate C code for the given schema into the target directory.
> > +def create_backend(path: str) -> QAPIBackend:
> > + if path is None:
> > + return QAPICBackend()
> >
> > - :param schema_file: The primary QAPI schema file.
> > - :param output_dir: The output directory to store generated code.
> > - :param prefix: Optional C-code prefix for symbol names.
> > - :param unmask: Expose non-ABI names through introspection?
> > - :param builtins: Generate code for built-in types?
> > + if "." not in path:
> > + print(f"Missing qualified module path in '{path}'", file=sys.stderr)
> > + sys.exit(1)
> >
> > - :raise QAPIError: On failures.
> > - """
> > - assert invalid_prefix_char(prefix) is None
> > + module_path, _, class_name = path.rpartition('.')
>
> I'd like to tweak this to
>
> module_path, dot, class_name = path.rpartition('.')
> if not dot:
> print(f"argument of -B must be of the form MODULE.CLASS",
> file=sys.stderr)
> sys.exit(1)
Yep, sure thing.
>
> > + try:
> > + mod = import_module(module_path)
> > + except Exception as ex:
>
> pylint complains:
>
> scripts/qapi/main.py:39:11: W0718: Catching too general exception Exception (broad-exception-caught)
>
> I can't see offhand what exception(s) we're supposed to catch here, so
> let's not worry about this now.
Yeah, I was concerned that by putting specialized exception
classes here, we would miss some possible scenarios, and IMHO
the completeness of catching Exception is better than the
technical purity of pylint's complaint.
> > + if not hasattr(mod, class_name):
> > + print(f"Module '{module_path}' has no class '{class_name}'", file=sys.stderr)
>
> pycodestyle-3 complains:
>
> scripts/qapi/main.py:44:80: E501 line too long (85 > 79 characters)
>
> Let's break the line after the comma, and also start the error message
> in lower case for consistency with error messages elsewhere.
>
> > + sys.exit(1)
> > + klass = getattr(mod, class_name)
>
> Alternatively
>
> try:
> klass = getattr(mod, class_name)
> except AttributeError:
> print(f"module '{module_path}' has no class '{class_name}'",
> file=sys.stderr)
> sys.exit(1)
>
> Admittedly a matter of taste. I tend to avoid the
>
> if frobnicate would fail:
> error out
> frobnicate
>
> pattern when practical.
I guess I tend to avoid using exception catching for such flow
control, but I don't mind that much either way.
> > - schema = QAPISchema(schema_file)
> > - gen_types(schema, output_dir, prefix, builtins)
> > - gen_features(schema, output_dir, prefix)
> > - gen_visit(schema, output_dir, prefix, builtins)
> > - gen_commands(schema, output_dir, prefix, gen_tracing)
> > - gen_events(schema, output_dir, prefix)
> > - gen_introspect(schema, output_dir, prefix, unmask)
> > + return backend
> > + except Exception as ex:
>
> Likewise too general exception.
>
> I'd like to shrink the try block and reduce the nesting:
>
> try:
> backend = klass()
> except Exception as ex:
> print(f"Backend '{path}' cannot be instantiated: {ex}", file=sys.stderr)
> sys.exit(1)
>
> if not isinstance(backend, QAPIBackend):
> print(f"Backend '{path}' must be an instance of QAPIBackend", file=sys.stderr)
> sys.exit(1)
>
> return backend
Sure, fine with me.
> Good enough.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> No need to respin, I can make the tweaks I suggested myself. Feel free
> to challenge my suggestions, of course.
Thank you, it is all fine.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Tue, Feb 25, 2025 at 01:31:56PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > The 'qapi.backend.QAPIBackend' class defines an API contract for code
>> > generators. The current generator is put into a new class
>> > 'qapi.backend.QAPICBackend' and made to be the default impl.
>> >
>> > A custom generator can be requested using the '-k' arg which takes a
>>
>> Missed an instance of -k. Can fix this myself.
>>
>> > fully qualified python class name
>> >
>> > qapi-gen.py -B the.python.module.QAPIMyBackend
>> >
>> > This allows out of tree code to use the QAPI generator infrastructure
>> > to create new language bindings for QAPI schemas. This has the caveat
>> > that the QAPI generator APIs are not guaranteed stable, so consumers
>> > of this feature may have to update their code to be compatible with
>> > future QEMU releases.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>
>> > -def generate(schema_file: str,
>> > - output_dir: str,
>> > - prefix: str,
>> > - unmask: bool = False,
>> > - builtins: bool = False,
>> > - gen_tracing: bool = False) -> None:
>> > - """
>> > - Generate C code for the given schema into the target directory.
>> > +def create_backend(path: str) -> QAPIBackend:
>> > + if path is None:
>> > + return QAPICBackend()
>> >
>> > - :param schema_file: The primary QAPI schema file.
>> > - :param output_dir: The output directory to store generated code.
>> > - :param prefix: Optional C-code prefix for symbol names.
>> > - :param unmask: Expose non-ABI names through introspection?
>> > - :param builtins: Generate code for built-in types?
>> > + if "." not in path:
>> > + print(f"Missing qualified module path in '{path}'", file=sys.stderr)
>> > + sys.exit(1)
>> >
>> > - :raise QAPIError: On failures.
>> > - """
>> > - assert invalid_prefix_char(prefix) is None
>> > + module_path, _, class_name = path.rpartition('.')
>>
>> I'd like to tweak this to
>>
>> module_path, dot, class_name = path.rpartition('.')
>> if not dot:
>> print(f"argument of -B must be of the form MODULE.CLASS",
>> file=sys.stderr)
>> sys.exit(1)
>
> Yep, sure thing.
>
>>
>> > + try:
>> > + mod = import_module(module_path)
>> > + except Exception as ex:
>>
>> pylint complains:
>>
>> scripts/qapi/main.py:39:11: W0718: Catching too general exception Exception (broad-exception-caught)
>>
>> I can't see offhand what exception(s) we're supposed to catch here, so
>> let's not worry about this now.
>
> Yeah, I was concerned that by putting specialized exception
> classes here, we would miss some possible scenarios, and IMHO
> the completeness of catching Exception is better than the
> technical purity of pylint's complaint.
Cleaner code would require a stronger contract.
We'll be just fine.
>> > + if not hasattr(mod, class_name):
>> > + print(f"Module '{module_path}' has no class '{class_name}'", file=sys.stderr)
>>
>> pycodestyle-3 complains:
>>
>> scripts/qapi/main.py:44:80: E501 line too long (85 > 79 characters)
>>
>> Let's break the line after the comma, and also start the error message
>> in lower case for consistency with error messages elsewhere.
>>
>> > + sys.exit(1)
>> > + klass = getattr(mod, class_name)
>>
>> Alternatively
>>
>> try:
>> klass = getattr(mod, class_name)
>> except AttributeError:
>> print(f"module '{module_path}' has no class '{class_name}'",
>> file=sys.stderr)
>> sys.exit(1)
>>
>> Admittedly a matter of taste. I tend to avoid the
>>
>> if frobnicate would fail:
>> error out
>> frobnicate
>>
>> pattern when practical.
>
> I guess I tend to avoid using exception catching for such flow
> control, but I don't mind that much either way.
I'm not a fan of using exceptions for mundane failures, but it's how
Python rolls.
>> > - schema = QAPISchema(schema_file)
>> > - gen_types(schema, output_dir, prefix, builtins)
>> > - gen_features(schema, output_dir, prefix)
>> > - gen_visit(schema, output_dir, prefix, builtins)
>> > - gen_commands(schema, output_dir, prefix, gen_tracing)
>> > - gen_events(schema, output_dir, prefix)
>> > - gen_introspect(schema, output_dir, prefix, unmask)
>> > + return backend
>> > + except Exception as ex:
>>
>> Likewise too general exception.
>>
>> I'd like to shrink the try block and reduce the nesting:
>>
>> try:
>> backend = klass()
>> except Exception as ex:
>> print(f"Backend '{path}' cannot be instantiated: {ex}", file=sys.stderr)
>> sys.exit(1)
>>
>> if not isinstance(backend, QAPIBackend):
>> print(f"Backend '{path}' must be an instance of QAPIBackend", file=sys.stderr)
>> sys.exit(1)
>>
>> return backend
>
> Sure, fine with me.
>
>
>> Good enough.
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>> No need to respin, I can make the tweaks I suggested myself. Feel free
>> to challenge my suggestions, of course.
>
> Thank you, it is all fine.
Cool, expect it in my next pull request. Thanks!
© 2016 - 2025 Red Hat, Inc.