From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57275) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDTWz-0000rS-8T for qemu-devel@nongnu.org; Thu, 16 Jun 2016 05:24:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDTWt-0002Jk-GO for qemu-devel@nongnu.org; Thu, 16 Jun 2016 05:24:08 -0400 From: Markus Armbruster References: <1464885987-4039-1-git-send-email-berrange@redhat.com> <1464885987-4039-3-git-send-email-berrange@redhat.com> <87eg86a27t.fsf@dusky.pond.sub.org> <20160614132553.GR4310@redhat.com> Date: Thu, 16 Jun 2016 11:23:57 +0200 In-Reply-To: <20160614132553.GR4310@redhat.com> (Daniel P. Berrange's message of "Tue, 14 Jun 2016 14:25:53 +0100") Message-ID: <87oa718p1u.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 02/11] qapi: allow QmpInputVisitor to auto-cast types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= , qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz "Daniel P. Berrange" writes: > On Thu, Jun 09, 2016 at 04:03:50PM +0200, Markus Armbruster wrote: >> "Daniel P. Berrange" 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 >> > --- >> > 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.