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

[Daniel P. Berrange]

On Tue, Jun 07, 2016 at 10:40:36AM -0600, Eric Blake wrote:
> 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.

If you want to have a shot, go ahead. I put this patch at the end of my
series, since it wasn't a blocking issue to resolve the struct field
ordering, just a "nice to have", so no rush.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|