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

[Daniel P. Berrange]

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.

> > +++ 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 <address@hidden>
> > + *
> > + * 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 :|