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