Re: [PATCH 8/9] scripts/qapi-gen: add -i option

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Aug 2, 2022 at 5:28 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Hi
>> >
>> >
>> > On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster <armbru@redhat.com> 
>> > wrote:

[...]

>> >> The option name --include doesn't really tell me what it is about.  Is
>> >> it an include path for schema files?  Or is it about including something
>> >> in generated C?  Where in generated C?
>> >>
>> >> The help text provides clues: "headers" suggests .h, and "top-level"
>> >> suggests somewhere top-like.
>> >>
>> >> In fact, it's about inserting C code at the beginning of generated .c
>> >> files.  For the uses we have in mind, the C code is a single #include.
>> >> The patch implements any number of #includes.
>> >>
>> >> More general and arguably less funky: a way to insert arbitrary C code.
>> >>
>> >> Except arbitrary C code on the command line is unwieldy.  Better kept it
>> >> in the schema.  Pragma?
>> >>
>> >> Thoughts?
>> >
>> > Pragmas are global currently. This doesn't scale well, as we would
>> > like to split the schemas. I have a following patch that will allow me
>> > to split/merge the pragmas. This is not optimal either, I would rather
>> > remove/replace them (using annotations).
>>
>> Now I'm curious.  Can you sketch what you have in mind?
>>
>
> I simply made the pragma lists additive:
>
> https://gitlab.com/marcandre.lureau/qemu/-/commit/1861964a317c2e74bea2d1f86944625e00df777f
>
>
> I didn't think much about replacing pragmas with extra annotations. But it
> could be for ex moving some pragmas to the declarations.
>
> From:
>
> { 'pragma': {
>     # Command names containing '_'
>     'command-name-exceptions': [
>         'add_client',
> ...
>
> { 'command': 'add_client',
>   'data': { ... } }
>
> To:
>
> { 'command': {
>     'name': 'add_client',
>     # Command name containing '_'
>     'name-exception': true },
>   'data': { ... } }
>
> Or eventually to the comment:
>
> # @add_client: (name-exception):

Keeping the QAPI rule violation overrides separate is kind of awkward,
but 1. it makes rule violations easy to spot in review, and 2. making
rule violations awkward helps deter people from violating the rules.

I figure the point of making pragmas additive is to let us avoid
duplication as we go from single schema to multiple schemas sharing
stuff.

We already do that for the storage daemon, admittedly in a crude &
stupid way.  We simply reuse the entire pragma.json.  Possible because
unused ones get ignored.

>> > Imho, global tweaking of compilation is better done from the command
>> > line.
>>
>> The command line is fine for straightforward configuration.  It's not
>> suitable for injecting code.
>>
>> Fine: cc -c, which tells the compiler to work in a certain way.
>>
>> Still fine: cc -DFOO, which effectively prepends '#define FOO 1" to the
>> .c.
>>
>> No longer fine: a hypothetical option to prepend arbitrary C code.  Even
>> if it was occasionally useful.
>>
>> Now watch this:
>>
>>     $ python qapi-gen.py -o t qapi/qapi-schema.json -i '"abc.h"
>>     #define FOO'
>>
>>     $ head -n 16 t/qapi-types.c
>>     /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>>
>>     /*
>>      * Schema-defined QAPI types
>>      *
>>      * Copyright IBM, Corp. 2011
>>      * Copyright (c) 2013-2018 Red Hat Inc.
>>      *
>>      * This work is licensed under the terms of the GNU LGPL, version 2.1
>> or later.
>>      * See the COPYING.LIB file in the top-level directory.
>>      */
>>
>>     #include "abc.h"
>>     #define FOO
>>
>>     #include "qapi/dealloc-visitor.h"
>>
>> Sure, nobody of sane mind would ever do this.  The fact remains that
>> we're doing something on the command line that should not be done there.
>>
>> Your -i enables code injection because it takes either a file name or a
>> #include argument.  Can we dumb it down to just file name?
>>
>>
> I think that can work too. Users can include a header that itself includes
> extra headers in different ways, if needed.

Yes.  It could even be named "qemu/osdep.h" ;)

Teasing aside, I'm okay with a simple option to override the name of the
header to include first.