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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 08/04/2015 09:58 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.
>
> I'm starting to be more in favor of this patch, both for ABI reasons and
> for size shavings.  It definitely looks better than in v2, where munged
> names are just an arbitrary number, and where builtins are not munged.

Yes, much easier to read, and smaller, too.  Review pays :)

>> TODO Either generate comments with the true type names, or provide an
>> option to generate without type name hiding.
>
> See also my comments on 30/32 about whether we should mask array types
> differently (and yes, the lack of comments made it a bit harder to chase
> down types for my commentary in that mail).
>
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  docs/qapi-code-gen.txt     | 14 +++++++-------
>>  scripts/qapi-introspect.py | 24 +++++++++++++++++++++---
>>  2 files changed, 28 insertions(+), 10 deletions(-)
>> 
>
> If we don't do anything to array names, then:
> Reviewed-by: Eric Blake <address@hidden>
>
> If we DO touch array names, I suspect this will look a lot different,
> and have to drop R-b for v4.
>
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index ed04770..3a78cf4 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -868,13 +868,13 @@ Example:
>>  [Uninteresting stuff omitted...]
>>  
>>      const char example_qmp_schema_json[] = "["
>> -        "{ \"arg-type\": \":empty\", \"name\": \"MY_EVENT\", \"meta-type\": 
>> \"event\" }, "
>> -        "{ \"meta-type\": \"builtin\", \"name\": \"int\", \"json-type\": 
>> \"int\" }, "
>> -        "{ \"meta-type\": \"builtin\", \"name\": \"str\", \"json-type\": 
>> \"string\" }, "
>> -        "{ \"meta-type\": \"object\", \"name\": \":empty\", \"members\": [ 
>> ] }, "
>> -        "{ \"meta-type\": \"object\", \"name\": \":obj-my-command-arg\", 
>> \"members\": [{ \"type\": \"UserDefOne\", \"name\": \"arg1\" } ] }, "
>> -        "{ \"meta-type\": \"object\", \"name\": \"UserDefOne\", 
>> \"members\": [{ \"type\": \"int\", \"name\": \"integer\" }, { \"type\": 
>> \"str\", \"name\": \"string\" } ] }, "
>> -        "{ \"arg-type\": \":obj-my-command-arg\", \"ret-type\": 
>> \"UserDefOne\", \"name\": \"my-command\", \"meta-type\": \"command\" } ]";
>> +        "{ \"arg-type\": \"0\", \"meta-type\": \"event\", \"name\": 
>> \"MY_EVENT\" }, "
>> +        "{ \"arg-type\": \"1\", \"meta-type\": \"command\", \"name\": 
>> \"my-command\", \"ret-type\": \"2\" }, "
>> +        "{ \"members\": [ ], \"meta-type\": \"object\", \"name\": \"0\" }, "
>> +        "{ \"members\": [{ \"name\": \"arg1\", \"type\": \"2\" } ], 
>> \"meta-type\": \"object\", \"name\": \"1\" }, "
>> +        "{ \"members\": [{ \"name\": \"integer\", \"type\": \"int\" }, { 
>> \"name\": \"string\", \"type\": \"str\" } ], \"meta-type\": \"object\", 
>> \"name\": \"2\" }, "
>> +        "{ \"json-type\": \"int\", \"meta-type\": \"builtin\", \"name\": 
>> \"int\" }, "
>> +        "{ \"json-type\": \"string\", \"meta-type\": \"builtin\", \"name\": 
>> \"str\" } ]";
>
> Some churn here once you fix the sorting in 30.
>
>> +
>> +    def _name(self, name):
>> +        if name not in self.name_map:
>> +            n = len(self.name_map)
>> +            self.name_map[name] = '%s' % n
>
> When string-izing an integer, isn't it better to use:
>
> self.name_map[name] = '%d' % len(self.name_map)

Yes, will change.

Thanks!