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

[Markus Armbruster <armbru@redhat.com>]

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!