Re: [Qemu-devel] [PATCH v5 02/11] qapi: allow QmpInputVisitor to auto-cast types

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Jun 09, 2016 at 04:03:50PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > Currently the QmpInputVisitor assumes that all scalar
> > values are directly represented as their final types.
> > ie it assumes an 'int' is using QInt, and a 'bool' is
> > using QBool.
> >
> > This extends it so that QString is optionally permitted
> > for any of the non-string scalar types. This behaviour
> > is turned on by requesting the 'autocast' flag in the
> > constructor.
> >
> > This makes it possible to use QmpInputVisitor with a
> > QDict produced from QemuOpts, where everything is in
> > string format.
> 
> We're still digging.
> 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  docs/qapi-code-gen.txt             |   2 +-
> >  include/qapi/qmp-input-visitor.h   |   6 +-
> >  qapi/opts-visitor.c                |   1 +
> >  qapi/qmp-input-visitor.c           |  89 ++++++++++++++++++++++------
> >  qmp.c                              |   2 +-
> >  qom/qom-qobject.c                  |   2 +-
> >  replay/replay-input.c              |   2 +-
> >  scripts/qapi-commands.py           |   2 +-
> >  tests/check-qnull.c                |   2 +-
> >  tests/test-qmp-commands.c          |   2 +-
> >  tests/test-qmp-input-strict.c      |   2 +-
> >  tests/test-qmp-input-visitor.c     | 115 ++++++++++++++++++++++++++++++++++++-
> >  tests/test-visitor-serialization.c |   2 +-
> >  util/qemu-sockets.c                |   2 +-
> >  14 files changed, 201 insertions(+), 30 deletions(-)


> > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> > index 4cf1cf8..00e4b1a 100644
> > --- a/qapi/opts-visitor.c
> > +++ b/qapi/opts-visitor.c
> > @@ -347,6 +347,7 @@ opts_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
> >      }
> >  
> >      if (opt->str) {
> > +        /* Keep these values in sync with same code in qmp-input-visitor.c */
> 
> Also with parse_option_bool().  That's the canonical place, in fact.

....except parse_option_bool() doesn't allow "yes", "no", "y", "n" as valid
values - it only permits "on" and "off" :-(  I guess I could make the
parse_option_bool() method non-static, make it accept these missing values,
and then call it from all places so we have guaranteed consistency.

> 
> >          if (strcmp(opt->str, "on") == 0 ||
> >              strcmp(opt->str, "yes") == 0 ||
> >              strcmp(opt->str, "y") == 0) {
> > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > index aea90a1..92023b1 100644
> > --- a/qapi/qmp-input-visitor.c
> > +++ b/qapi/qmp-input-visitor.c
> > @@ -20,6 +20,7 @@
> >  #include "qemu-common.h"
> >  #include "qapi/qmp/types.h"
> >  #include "qapi/qmp/qerror.h"
> > +#include "qemu/cutils.h"
> >  
> >  #define QIV_STACK_SIZE 1024
> >  
> > @@ -45,6 +46,7 @@ struct QmpInputVisitor
> >  
> >      /* True to reject parse in visit_end_struct() if unvisited keys remain. */
> >      bool strict;
> > +    bool autocast;
> >  };
> >  
> >  static QmpInputVisitor *to_qiv(Visitor *v)
> > @@ -254,15 +256,25 @@ static void qmp_input_type_int64(Visitor *v, const char *name, int64_t *obj,
> >                                   Error **errp)
> >  {
> >      QmpInputVisitor *qiv = to_qiv(v);
> > -    QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));
> > +    QObject *qobj = qmp_input_get_object(qiv, name, true);
> > +    QInt *qint;
> > +    QString *qstr;
> >  
> > -    if (!qint) {
> > -        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> > -                   "integer");
> > +    qint = qobject_to_qint(qobj);
> > +    if (qint) {
> > +        *obj = qint_get_int(qint);
> >          return;
> >      }
> >  
> > -    *obj = qint_get_int(qint);
> > +    qstr = qobject_to_qstring(qobj);
> > +    if (qstr && qstr->string && qiv->autocast) {
> > +        if (qemu_strtoll(qstr->string, NULL, 10, obj) == 0) {
> 
> In the commit message, you mentioned intended use: "with a QDict
> produced from QemuOpts, where everything is in string format."  I figure
> you mean opts with empty opts->list->desc[].  For those, we accept any
> key and parse all values as strings.
> 
> The idea with such QemuOpts is to *delay* parsing until we know how to
> parse.  The delay should be transparent to the user.  In particular,
> values should be parsed the same whether the parsing is delayed or not.
> 
> The canonical QemuOpts value parser is qemu_opt_parse().  It parses
> integers with parse_option_number().  That function parses like
> stroull(qstr->string, ..., 0).  Two differences:
> 
> * stroll() vs. strtoull(): they differ in ERANGE behavior.  This might
>   be tolerable, but I haven't thought it through.
> 
> * Base 0 vs 10: different syntax and semantics.  Oops.

Sigh, I originally had my code using strtoull() directly as the
parse_option_number() method does, but had to change it to make
checkpatch.pl stfu thus creating incosistency. This is a great
example of why I hate the fact that we only validate our style
rules against new patches and not our existing codebase :-(

Anyway, it seems to be something where we should refactor code to
use the same parsing method from both places. eg by making the
parse_option_number method non-static and just calling it from
here.




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 :|