Re: [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types

[Eric Blake]

On 06/07/2016 10:20 AM, Daniel P. Berrange wrote:
> On Tue, Jun 07, 2016 at 10:09:48AM -0600, Eric Blake wrote:
>> On 06/07/2016 04:11 AM, Daniel P. Berrange wrote:
>>> The current approach for pretty-printing QAPI types is to
>>> convert them to JSON using the QMP output visitor and then
>>> pretty-print the JSON document. This has an unfixable problem
>>> that structs get their keys printed out in random order, since
>>> JSON dicts do not contain any key ordering information.
>>>
>>> To address this, introduce a text output visitor that can
>>> directly pretty print a QAPI type into a string.
>>>
>>> Signed-off-by: Daniel P. Berrange <address@hidden>
>>> ---
>>>  include/qapi/text-output-visitor.h |  73 ++++++++++++
>>>  include/qapi/visitor-impl.h        |   5 +-
>>>  include/qapi/visitor.h             |   5 +-
>>>  qapi/Makefile.objs                 |   1 +
>>>  qapi/opts-visitor.c                |   5 +-
>>>  qapi/qapi-dealloc-visitor.c        |   4 +-
>>>  qapi/qapi-visit-core.c             |   9 +-
>>>  qapi/qmp-input-visitor.c           |   5 +-
>>>  qapi/qmp-output-visitor.c          |   4 +-
>>>  qapi/string-input-visitor.c        |   5 +-
>>>  qapi/string-output-visitor.c       |   5 +-
>>
>> Why can't we enhance the existing string-output-visitor to handle structs?
> 
> string-output-visitor seems to be doing something very
> different from this. In particular it only ever seems
> to output the values, never the field names. So if we
> did enhance string-output-visitor, we'd basically have
> to make all of its code conditional to output in one
> style or the other style, at which point I didn't think
> it was really buying us anything vs a new visitor.

That is, it was always doing a top-level visit of a scalar or array of
scalars, and nothing else. It may still be something that can be merged.
Maybe I should take a rough shot at it, since I have ideas on how to use
a common handler for name/list index (and do nothing at the top level),
then the rest of each callback is independent from what name prefix, if
any, was output.  On the other hand, I guess the way intList is handled
(compacting it into a single list, instead of each element of the list),
may indeed be a reason to keep it as two visitors.


>> Why do we need to list the element size twice?  Or is one the size of
>> the GenericList object wrapping the element?  I'm still not convinced we
>> need the size of an element at this point in the visit.
> 
> 'size_t element' isn't the size of an element, it is the really
> the list index - eg 1, 2, 3, 4, etc. I didn't call it 'index'
> because that causes a clashing symbol, but I guess I could
> have used 'idx' instead.

The perils of replying to email text in the order I read it. I think I
figured that out later on.  And it is indeed annoying that older gcc
warns about conflicts with shadowing 'index'.


>> Don't we already have a util/ function for pretty-printing a size?  In
>> fact, doesn't the existing StringOutputVisitor have code for doing it?
> 
> Guess where this code was copied from - StringOutputVisitor :-) If this
> idea is amenable in general, I'll go back and cleanup this patch be better.

A common helper may be nice, if we have two visitors both wanting to use it.


>>
>> Oooooh, I see.  You're using 'element' as the index within the array,
>> not as a size.  I think naming it 'idx' (or 'index', if that doesn't
>> cause older compilers to barf for shadowing a function name), would make
>> more sense, but it definitely highlights the need for documentation.
>> Also, why does element 0 have to be special-cased?
> 
> The pattern of calling means 'next_list' is invoked /after/
> each element is visited, but we want to print out the header
> /before/ each element. So I had to special case 0, and also
> use the 'if(ret)' check to skip printing a header after the
> last element.

But if we have a common helper function (see json_output_name() in my
JSON series), then that helper will be called before every element, and
can easily tell whether we are in struct context (name is non-null,
print it) or list context (name is NULL, print our current counter then
increment it), rather than having to special case the code around the
elements.

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

signature.asc
Description: OpenPGP digital signature