Re: [PATCH v2] qapi: pluggable backend code generators

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|