[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v1 4/6] qapi: add a text output visitor for pret
Re: [Qemu-block] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types
Tue, 7 Jun 2016 10:40:36 -0600
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0
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
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Description: OpenPGP digital signature
[Qemu-block] [PATCH v1 2/6] block: export LUKS specific data to qemu-img info, Daniel P. Berrange, 2016/06/07
[Qemu-block] [PATCH v1 6/6] block: convert to use qapi_stringify_ImageInfoSpecific, Daniel P. Berrange, 2016/06/07
[Qemu-block] [PATCH v1 5/6] qapi: generate a qapi_stringify_TYPENAME method for all types, Daniel P. Berrange, 2016/06/07
Re: [Qemu-block] [PATCH v1 0/6] Report format specific info for LUKS block driver, Eric Blake, 2016/06/07