[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection |
Date: |
Wed, 29 Jul 2015 19:26:12 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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 :)
- Re: [Qemu-devel] [PATCH RFC v2 33/47] qapi: Clean up after recent conversions to QAPISchemaVisitor, (continued)
[Qemu-devel] [PATCH RFC v2 44/47] qapi: Pseudo-type '**' is now unused, drop it, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 40/47] qapi: Introduce a first class 'any' type, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 36/47] qapi: Rename qmp_marshal_input_FOO() to qmp_marshal_FOO(), Markus Armbruster, 2015/07/01