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 <berra...@redhat.com>
> > ---
> > 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.
> > +++ b/include/qapi/visitor-impl.h
> > @@ -52,10 +52,11 @@ struct Visitor
> > /* Must be set; implementations may require @list to be non-null,
> > * but must document it. */
> > void (*start_list)(Visitor *v, const char *name, GenericList **list,
> > - size_t size, Error **errp);
> > + size_t size, bool hasElements, Error **errp);
>
> Ah, you had to touch ALL the visitors, to add hasElements. What are its
> semantics on an input list, where you don't know if you have elements
> until after calling visit_start_list()? If we DO need this parameter,
> it should be a separate patch to add it in, then the patch to create the
> new visitor. But I'm not convinced we need it: *list already serves
> that purpose (if *list is NULL, you have no elements; if it is non-NULL,
> then you have at least one element; and if the list is optional, then
> the call to visit_type_optional() already tells you whether the
> [possibly-empty] list is even present).
Yep, I didn't realize that *list could be used to infer this.
This addition is clearly redundant based on that.
>
> >
> > /* Must be set */
> > - GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
> > + GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size,
> > + size_t element);
>
> 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.
> > +++ b/qapi/text-output-visitor.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * Text pretty printing Visitor
> > + *
> > + * Copyright Red Hat, Inc. 2016
> > + *
> > + * Author: Daniel Berrange <berra...@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or
> > later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu-common.h"
> > +#include "qapi/text-output-visitor.h"
> > +#include "qapi/visitor-impl.h"
> > +#include <math.h>
> > +
> > +struct TextOutputVisitor {
> > + Visitor visitor;
> > + GString *string;
> > + int level;
> > + int skipLevel;
> > + int extraIndent;
> > +};
> > +
> > +static TextOutputVisitor *to_sov(Visitor *v)
> > +{
> > + return container_of(v, TextOutputVisitor, visitor);
> > +}
> > +
> > +
> > +#define INDENT (sov->extraIndent + ((sov->level - sov->skipLevel) * 4))
> > +
> > +static void print_type_int64(Visitor *v, const char *name, int64_t *obj,
> > + Error **errp)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > +
> > + if (sov->level < sov->skipLevel) {
> > + return;
> > + }
> > + g_string_append_printf(sov->string,
> > + "%*s%s: %" PRIu64 "\n",
> > + INDENT, "",
> > + name, *obj);
>
> name is NULL during a list visit, and directly printing NULL through %s
> is a glibc extension, liable to crash elsewhere.
Oh of course - I only tested where the list elements were themselves
structs, so missed that nuance.
> > +static void print_type_uint64(Visitor *v, const char *name, uint64_t *obj,
> > + Error **errp)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > +
> > + if (sov->level < sov->skipLevel) {
> > + return;
> > + }
> > + g_string_append_printf(sov->string,
> > + "%*s%s: %" PRIi64 "\n",
> > + INDENT, "",
> > + name, *obj);
>
> And again. You may want to copy ideas from my JSON output visitor, for
> having common code that outputs the indentation and name (when present),
> rather than duplicating the common part in every callback.
>
> > +}
> > +
> > +static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
> > + Error **errp)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > + static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
>
> 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.
> > +static GenericList *next_list(Visitor *v, GenericList *tail,
> > + size_t size, size_t element)
> > +{
> > + TextOutputVisitor *sov = to_sov(v);
> > + GenericList *ret = tail->next;
> > + if (ret && sov->level >= sov->skipLevel) {
> > + g_string_append_printf(sov->string,
> > + "%*s[%zu]:\n",
> > + INDENT, "", element);
>
> 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.
> Is there a way to
> get the element number embedded into the 'name' parameter, rather than
> our current practice of passing NULL for the name? Is there any way to
> track the state in a stack, rather than making the caller pass in a new
> parameter, so that printing the index number is localized to this
> visitor rather than touching the interface to every other visitor?
That's a potential approach I can look at to avoid adding the
extra parameter. It would still need the first/last special
case. I'm totally open minded about solutions really - this
was simply the quickest possible approach to demonstrate the
problem i was solving. Happy to redo it with any better
approach
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 :|
