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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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?

Interesting idea, definitely worth a doc search.

Prettier output can of course be punted to a followup-patch.

>>>                              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.

Got it.

>>> 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.

Agree.

If sorting everything turns out to be cheap (in complexity; other
metrics don't matter much), I'm inclined to sort.

>>>> 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 :)