Re: [Qemu-devel] [PATCH RFC v4 32/32] qapi-introspect: Hide type names

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/03/2015 08:30 AM, Markus Armbruster wrote:
>> To eliminate the temptation for clients to look up types by name
>> (which are not ABI), replace all type names by meaningless strings.
>> 
>> Reduces output of query-schema by 13 out of 85KiB.
>> 
>> As a debugging aid, provide option -u to suppress the hiding.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  docs/qapi-code-gen.txt     | 36 +++++++++++++++++-------------------
>>  qapi/introspect.json       | 11 +++--------
>>  scripts/qapi-introspect.py | 41 +++++++++++++++++++++++++++++++++++------
>>  3 files changed, 55 insertions(+), 33 deletions(-)
>> 
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index a079b51..94fc296 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -516,13 +516,16 @@ additional variant members depending on the
>> value of meta-type.
>>  Each SchemaInfo object describes a wire ABI entity of a certain
>>  meta-type: a command, event or one of several kinds of type.
>>  
>> -SchemaInfo for entities defined in the QAPI schema have the same name
>> -as in the schema.  This is the case for all commands and events, and
>> -most types.
>> +SchemaInfo for commands and events have the same name as in the QAPI
>> +schema
>
> Missing a trailing '.'

Will fix.

>>  Command and event names are part of the wire ABI, but type names are
>> -not.  Therefore, looking up a type by its name in the QAPI schema is
>> -wrong.  Look up the command or event, then follow references by name.
>> +not.  Therefore, the SchemaInfo for types have auto-generated
>> +meaningless names.  For readability, the examples in this document use
>> +meaningful type names instead.
>> +
>> +To examine a type, start with a the command or event using it, then
>
> s/a the/the/

Will fix.

>> +follow references by name.
>>  
>
>> @@ -1053,13 +1051,13 @@ Example:
>>  [Uninteresting stuff omitted...]
>>  
>>      const char example_qmp_schema_json[] = "["
>> -        "{\"arg-type\": \":empty\", \"meta-type\": \"event\", \"name\": 
>> \"MY_EVENT\"}, "
>> +        "{\"arg-type\": \"0\", \"meta-type\": \"event\", \"name\": 
>> \"MY_EVENT\"}, "
>> +        "{\"arg-type\": \"1\", \"meta-type\": \"command\", \"name\": 
>> \"my-command\", \"ret-type\": \"2\"}, "
>
> I thought you said that this document would use the meaningful type
> names?  Oh, I see - the earlier examples (not touched by this commit) DO
> use meaningful names; while _this_ example uses the munged names. I
> don't know if any additional wording would help.

Different section.  I'll change the wording there

    meaningless names.  For readability, the examples in this document use
    meaningful type names instead.

to say "this section" instead of "this document".

> Would it be worth documenting -u as a usage argument for
> qapi-introspect.py?  [For that matter, should we document what -m for
> middle-mode does for qapi-commands.py? But as a separate patch]

None of the options are documented outside the source, and even the
source doesn't really explain.  It's just a development tool.  I guess
adding --help would be nice.  Beyond that, returns diminish rapidly.

> I still think that masking arrays as "[123]" is nicer, but that can be a
> followup patch (especially since I've already posted it in one of my
> numerous RFC followups, which I'll have to rebase anyways)
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!