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

[Markus Armbruster <armbru@redhat.com>]

"Daniel P. Berrange" <berrange@redhat.com> writes:

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

Or factor out its parsing guts and put them into cutils.c.  Similar to
how qemu_strtol() & friends are (or should be) the common number
parsers.

But that's optional.  All I'm asking for this patch is an updated
comment.

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

To get this patch accepted, you need to fix the inconsistency.
Refactoring to improve our chances the inconsistency stays fixed is
welcome, but optional.