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 <[email protected]> > --- > 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
