qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v1 4/6] qapi: add a text output visitor for pretty printing types
Date: Tue, 7 Jun 2016 10:09:48 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

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?

>  qapi/text-output-visitor.c         | 235 
> +++++++++++++++++++++++++++++++++++++
>  scripts/qapi-visit.py              |   5 +-

Missing a testsuite addition.

>  13 files changed, 339 insertions(+), 22 deletions(-)
>  create mode 100644 include/qapi/text-output-visitor.h
>  create mode 100644 qapi/text-output-visitor.c
> 
> diff --git a/include/qapi/text-output-visitor.h 
> b/include/qapi/text-output-visitor.h
> new file mode 100644
> index 0000000..3b742b7
> --- /dev/null
> +++ b/include/qapi/text-output-visitor.h
> @@ -0,0 +1,73 @@

> +TextOutputVisitor *text_output_visitor_new(int extraIndent,
> +                                           int skipLevel);

Conflicts with the work in my JSON/cloning visitor series, which tries
to hide all visitors behind a polymorphic interface so that we don't
need to expose the subtype TextOutputVisitor to the caller.


> +++ 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).

>  
>      /* 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.


> +++ b/include/qapi/visitor.h
> @@ -303,7 +303,7 @@ void visit_end_struct(Visitor *v);

Missing documentation about the new visitor in the up-front summary
documentation.

>   * even if intermediate visits fail.  See the examples above.
>   */
>  void visit_start_list(Visitor *v, const char *name, GenericList **list,
> -                      size_t size, Error **errp);
> +                      size_t size, bool hasElements, Error **errp);

Missing documentation on what the new parameter represents.

>  
>  /*
>   * Iterate over a GenericList during a non-virtual list visit.
> @@ -319,7 +319,8 @@ void visit_start_list(Visitor *v, const char *name, 
> GenericList **list,
>   * the list, with that function's name parameter set to NULL and obj
>   * set to the address of @tail->value.
>   */
> -GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
> +GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size,
> +                             size_t element);
>  

Missing documentation on what the new parameter represents.


> +++ 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.

> +}
> +
> +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?


> +static void print_type_number(Visitor *v, const char *name, double *obj,
> +                              Error **errp)
> +{
> +    TextOutputVisitor *sov = to_sov(v);
> +
> +    if (sov->level < sov->skipLevel) {
> +        return;
> +    }
> +    g_string_append_printf(sov->string,
> +                           "%*s%s: %f\n",

I guess %f is okay for human output (we already have documented bugs to
fix in our QMP output visitors that a default of %f loses precision).

> +static void
> +start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> +           bool hasElements, Error **errp)
> +{
> +    TextOutputVisitor *sov = to_sov(v);
> +
> +    if (sov->level >= sov->skipLevel) {
> +        g_string_append_printf(sov->string,
> +                               "%*s%s:\n",
> +                               INDENT, "",
> +                               name);
> +    }
> +    sov->level++;
> +    if (hasElements) {

Again, I think hasElements is redundant; you can use *list == NULL as a
witness of whether there are elements.

> +        if (sov->level >= sov->skipLevel) {
> +            g_string_append_printf(sov->string,
> +                                   "%*s[0]:\n",
> +                                   INDENT, "");
> +        }
> +    }
> +}
> +
> +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?  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?

> +++ b/scripts/qapi-visit.py
> @@ -115,14 +115,15 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> *name, %(c_name)s **obj, Error
>      Error *err = NULL;
>      %(c_name)s *tail;
>      size_t size = sizeof(**obj);
> +    size_t element = 0;
>  
> -    visit_start_list(v, name, (GenericList **)obj, size, &err);
> +    visit_start_list(v, name, (GenericList **)obj, size, *obj != NULL, &err);


This deferences *obj unconditionally, which may not be safe for input
visitors.

>      if (err) {
>          goto out;
>      }
>  
>      for (tail = *obj; tail;
> -         tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size)) 
> {
> +         tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size, 
> ++element)) {

Okay, this change may be independently useful, but again, I think
'index' (or 'idx') might be a nicer name than 'element' to convey what
it is tracking.  I also wonder if embedding it into 'name' makes more sense.

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]