"Daniel P. Berrange" <berra...@redhat.com> writes:
> On Thu, Jun 09, 2016 at 04:03:50PM +0200, Markus Armbruster wrote:
>> "Daniel P. Berrange" <berra...@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 <berra...@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.
