Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection

[Eric Blake]

On 07/29/2015 03:19 AM, Markus Armbruster wrote:
>>> Longest line is a bit over 4KiB for me.
>>>
>>
>> If we break up string literals, at least use some indentation to make it
>> obvious that multiple lines merge to a single array entry. For example
>> (after patch 47):
>>
>> ...
>>     "{ 'name': ':abr', 'meta-type': 'object', "
>>       "'members': [ "
>>         "{ 'name': 'device', 'type': ':acg', 'default': null }, "
>>         "{ 'name': 'node-name', 'type': ':acg', 'default': null }, "
>>         "{ 'name': 'snapshot-file', 'type': ':acg' }, "
>>         "{ 'name': 'snapshot-node-name', 'type': ':acg', 'default': null
>> }, "
>>         "{ 'name': 'format', 'type': ':acg', 'default': null }, "
>>         "{ 'name': 'mode', 'type': ':afo', 'default': null } ] }, "
>>     "{ 'name': ... "
> 
> Unconventional indentation, but if it helps the reader...

I'm not a stickler about the particular spacing I used, so much as
demonstrating an idea.  Pick any indentation you like; I was just
demonstrating that some well-chosen line breaks, coupled with visual
clues on what belongs together, can help in reading the string literal
in the generated file.

In fact, doesn't python have a way to pretty-print JSON, and then
post-process the pretty-printed string to add C \" escaping?

>>                              And if we don't want sorting, documenting
>> that data is NOT guaranteed to be position-dependent, in spite of being
>> in a JSON array, is a nice touch.
> 
> What do you mean by "position-dependent"?

If qemu 2.5 has { 'struct':'Foo', 'data': { 'b': 'int', 'c': 'int' } },
then qemu 2.6 adds '*a': 'int' to the end of that list, then either we
guarantee sorting (if you read the members and see 'b' first, then you
know 'a' was not added) or we don't (you must read the entire list to
see if 'a' has been added; and you cannot assume that 'a' will be last
even if it was listed last in the .json file of 2.6); the position in
the .json file need not determine the position in the introspection output.

>> But, as with enum sorting, actually documenting our choice will help
>> cement the expectations of clients on what they have to do when learning
>> if a parameter was added.
> 
> We may want to adopt a consistent rule on sorting stuff.

To be consistent, the rule would be that either everything is sorted, or
nothing is; and if we choose nothing to be sorted, we are unlikely to
ever want to add sorting in the future.

>>> I could shoehorn both views into a single visitor function, by passing
>>> both views, .base + .local_members, and .members.  All implementations
>>> will use only one of the views, but it's not immediately obvious which
>>> one.  So I chose to have two visitor functions.  Matter of taste.
>>
>> I can live with it (documentation that sub-classes should override at
>> most only one of the two visitors might help the cause, though).
> 
> Actually, overriding both would be just fine.  We just haven't had a use
> for that.

I guess it's hard to predict why a visitor would want to have both
callbacks called for every object, so we can't outright state that it is
useless. The whole point of a visitor interface is to provide
flexibility in designing a visitor later down the road :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature