On 03/10/2016 11:59 AM, Daniel P. Berrange wrote: > 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. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > include/qapi/qmp-input-visitor.h | 3 + > qapi/qmp-input-visitor.c | 96 +++++++++++++++++++++++++++----- > tests/test-qmp-input-visitor.c | 115 > ++++++++++++++++++++++++++++++++++++++- > 3 files changed, 196 insertions(+), 18 deletions(-) > > diff --git a/include/qapi/qmp-input-visitor.h > b/include/qapi/qmp-input-visitor.h > index 3ed499c..c25cb7c 100644 > --- a/include/qapi/qmp-input-visitor.h > +++ b/include/qapi/qmp-input-visitor.h > @@ -21,6 +21,9 @@ typedef struct QmpInputVisitor QmpInputVisitor; > > QmpInputVisitor *qmp_input_visitor_new(QObject *obj); > QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj); > +QmpInputVisitor *qmp_input_visitor_new_full(QObject *obj, > + bool strict, > + bool autocast);
We have so few uses of qmp_input_visitor_new* that it might be worth just having a single prototype, and maybe using an 'int flags' instead of a string of bool. But not a show-stopper for this patch (rather, an idea for a future patch). > - *obj = qint_get_int(qint); > + qstr = qobject_to_qstring(qobj); > + if (qstr && qstr->string && qiv->autocast) { > + errno = 0; Dead setting of errno, since... > + if (qemu_strtoll(qstr->string, NULL, 10, obj) == 0) { qemu_strtoll() handles it on your behalf, and you aren't using error_setg_errno(). > @@ -233,30 +245,61 @@ static void qmp_input_type_uint64(Visitor *v, const > char *name, uint64_t *obj, > { > /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ > 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) { > + errno = 0; > + if (qemu_strtoull(qstr->string, NULL, 10, obj) == 0) { And again. Hmm. Do we need to worry about partial asymmetry? That is, qint_get_int() returns a signed number, but qemu_strtoull() parses unsigned; if the original conversion from JSON to qint uses a different parser, then we could have funny results where we get different results for things like: "key1":9223372036854775807, "key2":"9223372036854775807", even though the same string of digits is being parsed, based on whether the different parsers handle numbers larger than INT64_MAX differently. [Ultimately, I'd like QInt to be enhanced to track whether the input was signed or unsigned, and automatically make the output match the input when converting back to string; that is, track 65 bits of information instead of 64; but that's no sooner than 2.7 material] > static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj, > Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > - QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true)); > + QObject *qobj = qmp_input_get_object(qiv, name, true); > + QBool *qbool; > + QString *qstr; > > - if (!qbool) { > - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > - "boolean"); > + qbool = qobject_to_qbool(qobj); > + if (qbool) { > + *obj = qbool_get_bool(qbool); > return; > } > > - *obj = qbool_get_bool(qbool); > + > + qstr = qobject_to_qstring(qobj); > + if (qstr && qstr->string && qiv->autocast) { > + if (!strcasecmp(qstr->string, "on") || > + !strcasecmp(qstr->string, "yes") || > + !strcasecmp(qstr->string, "true")) { > + *obj = true; > + return; > + } Do we also want to allow "0"/"1" for true/false? Overall, I'm a big fan of this patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature