Re: [Qemu-devel] [PATCH RFC v2 44/47] qapi: Pseudo-type '**' is now unused, drop it

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> 'gen': false needs to stay for now, because netdev_add is still using
>> it.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  docs/qapi-code-gen.txt                               | 15 ++++-----------
>>  scripts/qapi.py                                      | 20 
>> ++++----------------
>>  tests/Makefile                                       |  4 ++--
>>  tests/qapi-schema/args-returns-any.err               |  1 +
>>  ...type-bypass-no-gen.exit => args-returns-any.exit} |  0
>
>> 
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index 2367c66..ca578dd 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>
>> @@ -457,13 +454,9 @@ which would validate this Client JSON Protocol 
>> transaction:
>>  
>>  In rare cases, QAPI cannot express a type-safe representation of a
>>  corresponding Client JSON Protocol command.  In these cases, if the
>> -command expression includes the key 'gen' with boolean value false,
>> -then the 'data' or 'returns' member that intends to bypass generated
>> -type-safety and do its own manual validation should use an inline
>> -dictionary definition, with a value of '**' rather than a valid type
>> -name for the keys that the generated code will not validate.  Please
>> -try to avoid adding new commands that rely on this, and instead use
>> -type-safe unions.  For an example of bypass usage:
>> +command expression includes the key 'gen' with boolean value false.
>
> Incomplete sentence.  "if the command expression includes... false"
> needs "then something...".

Oops.  I'll rephrase.

>> +Please try to avoid adding new commands that rely on this, and instead
>> +use type-safe unions.  For an example of bypass usage:
>
> And given my ideas, we may be able to make netdev_add typesafe and
> eliminate 'gen' altogether in the future :)

The harder problem is qapifying device_add without use of 'gen': false.
It's structurally just like netdev_add, "only" with has many, many more
variants (the device models), which are collected only at run time.

Changing things to define device model properties in the schema seems
unappealing.

We could try to collect them at build time.  Not trivial.

We could collect them at run time, but then the schema becomes dynamic.
Not trivial, either.

The easiest solution is probably something like Python's **kwds
parameter: collect any remaining arguments in a dictionary, and pass
that as argument kwds.  Sadly, QAPI introspection remains useless for
device models then.

>> +++ b/tests/Makefile
>> @@ -222,11 +222,11 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, 
>> \
>>      bad-type-dict.json double-data.json unknown-expr-key.json \
>>      redefined-type.json redefined-command.json redefined-builtin.json \
>>      redefined-event.json command-int.json bad-data.json event-max.json \
>> -    type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \
>> +    type-bypass-bad-gen.json \
>
> If I understand correctly (git didn't quite guess right),
> type-bypass-no-gen.json disappears completely (we no longer have working
> '**'), and type-bypass.json...
>
>>      data-array-empty.json data-array-unknown.json data-int.json \
>>      data-unknown.json data-member-unknown.json data-member-array.json \
>>      data-member-array-bad.json args-union.json args-alternate.json \
>> -    returns-array-bad.json returns-int.json \
>> +    args-returns-any.json returns-array-bad.json returns-int.json \
>
> ...gets renamed args-returns-any.json (because type bypass is now
> expressed via 'any'.

I'd view args-returns-any.json as new.  But viewing it as a replacement
for type-bypass.json is okay.

>> +# built-in type 'any' in arguments and returns
>> +# works except for 'data': 'any', because that has to be a struct
>> +# note: command name 'qom-get' chosen to avoid "cannot use built-in" error
>> +{ 'command': 'qom-get', 'data': { 'arg': 'any' }, 'returns': 'any' }
>> +{ 'command': 'foo', 'returns': { 'arg': 'any' } }
>> +{ 'command': 'bar', 'data': 'any' }
>
> This shows what the parser accepts and rejects, but doesn't prove it
> compiles to C; and the previous patch that touched
> test-qmp-input-visitor.c was testing that the low-level interactions
> worked but did not map it all the way back to qapi.  I wonder if you
> should also enhance qapi-schema-test to use 'any', since that tends to
> be the one test that most fully exercises interactions with generated
> code based on a valid qapi schema.

I guess I'll have to split args-returns-any.json: move the positive test
cases to qapi-schema-test.json, move the negative test case to its own
test.

> On the right track, but I want to make sure the botched documentation is
> correct before giving R-b.

Thanks!