Re: [Qemu-devel] [PATCH RFC v2 47/47] qapi-introspect: Hide type names

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 07/01/2015 02:22 PM, 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 9 out of 80KiB.
>
> I'm not sure whether I like this or not.  It does make sense from the
> perspective of forcing clients to stick to ABI queries, but makes it a
> bit harder to navigate things except by automated scripts.

Yes.  I'm not sure it's a good idea.  If we decide to hide types this
way, then I'd find an option to generate without type hiding useful.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-introspect.py | 27 +++++++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
>> 
>> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
>> index 961fe88..efb34ff 100644
>> --- a/scripts/qapi-introspect.py
>> +++ b/scripts/qapi-introspect.py
>> @@ -9,13 +9,18 @@
>>  # This work is licensed under the terms of the GNU GPL, version 2.
>>  # See the COPYING file in the top-level directory.
>>  
>> +import string
>>  from qapi import *
>>  
>> +def _b32digit(num):
>> +    return (string.lowercase + string.digits[2:])[num]
>
> This feels a bit too magic for me to decipher late at night.
>
>> +
>>  class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
>>      def __init__(self):
>>          self.schema = None
>>          self.jsons = None
>>          self.used_types = None
>> +        self.name_map = None
>>          self.defn = None
>>          self.decl = None
>>  
>> @@ -23,13 +28,17 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
>>          self.schema = schema
>>          self.jsons = []
>>          self.used_types = []
>> +        self.name_map = {}
>>          return QAPISchemaType   # don't visit types for now
>>  
>>      def visit_end(self):
>>          # visit the types that are actually used
>> +        jsons = self.jsons
>> +        self.jsons = []
>>          for typ in self.used_types:
>>              typ.visit(self)
>>          self.jsons.sort()
>> +        jsons.extend(self.jsons)
>
> I'm not sure I follow the use of .extends() here.

jsons.extend(self.jsons) appends the elements of array self.jsons to
array jsons.

Before the patch:

At function entry, self.jsons contains introspection information for the
non-types.

The loop visits the types.  This adds introspection information for the
types to self.json.  Sort self.jsons by name.

What the patch adds:

Move the introspection information for the non-types out of the way
before the loop, append the information on types afterwards.  The result
is now in jsons rather than self.jsons (see the next patch hunk).

Why it does that:

With the funny typenames, sorting everything by name results in a mess.
Keeping non-types and types separate is less of a mess.

>>          name = prefix + 'qmp_schema_json'
>>          self.decl = mcgen('''
>>  extern char %(c_name)s[];
>> @@ -40,10 +49,19 @@ char %(c_name)s[] = "["
>>      "%(c_jsons)s]";
>>  ''',
>>                            c_name=c_name(name),
>> -                          c_jsons=', "\n    "'.join(self.jsons))
>> +                          c_jsons=', "\n    "'.join(jsons))
>>          self.schema = None
>>          self.jsons = None
>>          self.used_types = None
>> +        self.name_map = None
>> +
>> +    def _name(self, name):
>> +        if name not in self.name_map:
>> +            n = len(self.name_map)
>> +            self.name_map[name] = ':' + _b32digit(n / 32 / 32) \
>> +                                  + _b32digit(n / 32 % 32) \
>> +                                  + _b32digit(n % 32)
>
> Caps our qapi to 32k types (including implicit ones); will that be an issue?

I doubt it :)

Should we ever hit the limit, we can simply add another base32
character.

>> +        return self.name_map[name]
>>  
>>      def _use_type(self, typ):
>>          # Map the various integer types to plain int
>> @@ -55,9 +73,14 @@ char %(c_name)s[] = "["
>>          # Add type to work queue if new
>>          if typ not in self.used_types:
>>              self.used_types.append(typ)
>> -        return typ.name
>> +        # Clients should examine commands and events, not types.  Hide
>> +        # type names to reduce the temptation.  Also saves a few
>> +        # characters.
>> +        return self._name(typ.name)
>>  
>>      def _gen_json(self, name, mtype, extra):
>> +        if mtype != 'command' and mtype != 'event':
>> +            name = self._name(name)
>>          self.jsons.append("{ 'name': '%s', 'meta-type': '%s', %s }"
>>                            % (name, mtype, extra))
>>  
>>